cloudimpl: handle special char usernames in userfile

[adityamaru]

This change adds support for usernames that contain special
characters when using userfile. Usernames are used in the table name prefix for
the default target userfile operates on. For the table name to be valid
usernames with special characters need to be quoted.

Fixes: #58430

Release note (bug fix): Usernames with special characters would not work
with userfile since they were not appropriately quoted.

[cockroach-teamcity]

This change is 

[knz]

The approach you've taken is still not correct, see my example test cases you should be using.

Also you're repeating the code in multiple places -- is there a way to ensure the proper implementation is present in just 1 place, then reused everywhere it needs to be?

Reviewed 5 of 6 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @dt)

---

pkg/cli/userfile.go, line 162 at r1 (raw file):

	lex.EncodeUnrestrictedSQLIdent(&sqlUsername, defaultUserfileTablePrefix+user,
		lex.EncNoFlags)
	return fmt.Sprintf("%s.%s", defaultUserfileDbAndSchema, sqlUsername.String())

This encoding method is incorrect. You should use tree.TableName, populate the fields and and call String on that.

---

pkg/cli/userfile.go, line 363 at r1 (raw file):

	targetTable := qualifiedTableName + fileTableNameSuffix
	if strings.HasSuffix(qualifiedTableName, "\"") {
		targetTable = fmt.Sprintf("%s%s\"", strings.TrimSuffix(qualifiedTableName, "\""),

This encoding method is incorrect. You should use tree.TableName, populate the fields and and call String on that.

---

pkg/cli/userfiletable_test.go, line 511 at r1 (raw file):

			},
			{
				"special-char-username",

please add the following strings too:

"this.username",
`this"username`,
`this""username`,
`this\044username`,
`this\xXXusername`,

---

pkg/storage/cloudimpl/file_table_storage.go, line 59 at r1 (raw file):

	lex.EncodeUnrestrictedSQLIdent(&sqlUsername, defaultUserfileTablePrefix+user,
		lex.EncNoFlags)
	return fmt.Sprintf("%s.%s", defaultUserfileDbAndSchema, sqlUsername.String())

This encoding method is incorrect. You should use tree.TableName, populate the fields and and call String on that.

---

pkg/storage/cloudimpl/filetable/file_table_read_writer.go, line 184 at r1 (raw file):

func maybeConstructQuotedTableName(tablePrefix, tableSuffix string) string {
	if strings.HasSuffix(tablePrefix, "\"") {
		return fmt.Sprintf("%s%s\"", strings.TrimSuffix(tablePrefix, "\""), tableSuffix)

This encoding method is incorrect. You should use tree.TableName, populate the fields and and call String on that.

[adityamaru]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @knz)

---

pkg/cli/userfile.go, line 162 at r1 (raw file):

Previously, knz (kena) wrote…

This encoding method is incorrect. You should use tree.TableName, populate the fields and and call String on that.

Done.

---

pkg/cli/userfile.go, line 363 at r1 (raw file):

Previously, knz (kena) wrote…

This encoding method is incorrect. You should use tree.TableName, populate the fields and and call String on that.

This is actually a weird one. If qualifiedTableName is a quoted name egs: defaultdb.public."foo-foo", what we want to do is extend this to "defaultdb.public."foo-foo_upload_files". I don't think tree.TableName can help us in this case?

---

pkg/cli/userfiletable_test.go, line 511 at r1 (raw file):

Previously, knz (kena) wrote…

please add the following strings too:

"this.username",
`this"username`,
`this""username`,
`this\044username`,
`this\xXXusername`,

The latter 4 are all invalid usernames as far as the rules for valid usernames go, are you suggesting expecting errors on these cases?

---

pkg/storage/cloudimpl/file_table_storage.go, line 59 at r1 (raw file):

Previously, knz (kena) wrote…

This encoding method is incorrect. You should use tree.TableName, populate the fields and and call String on that.

Done.

---

pkg/storage/cloudimpl/filetable/file_table_read_writer.go, line 184 at r1 (raw file):

Previously, knz (kena) wrote…

This encoding method is incorrect. You should use tree.TableName, populate the fields and and call String on that.

Same explanation as above.

[adityamaru]

@knz apologies this fell down the list of things todo, but I've cleaned it up and left a few comments, I'd appreciate another look 🙂

[knz]

Much better thanks! just one comment left

Reviewed 1 of 6 files at r1, 4 of 4 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @dt)

---

pkg/cli/userfile.go, line 363 at r1 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

This is actually a weird one. If qualifiedTableName is a quoted name egs: defaultdb.public."foo-foo", what we want to do is extend this to "defaultdb.public."foo-foo_upload_files". I don't think tree.TableName can help us in this case?

1. make a TableName
2. add the suffix _upload_files to your tablename's .ObjectName
3. call String on the TableName to get the properly quoted result.

---

pkg/cli/userfiletable_test.go, line 511 at r1 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

The latter 4 are all invalid usernames as far as the rules for valid usernames go, are you suggesting expecting errors on these cases?

these are the rules for SQL-defined usernames but 1) we may change them tomorrow 2) we are soon going to accept username definitions from outside of SQL which may have different rules.

I think the test that would be nice to see is how the names are derived for identifiers that contain arbitrary characters, regardless of whether they are currently valid SQL users.

Also, I forgot an example:

family (or index)

these two usernames are valid according to current rules but they are also reserved keywords, and so the test might reveal some additional quoting is necessary.

---

pkg/storage/cloudimpl/filetable/file_table_read_writer.go, line 832 at r2 (raw file):

		lex.EncodeRestrictedSQLIdent(&quotedUsername, user, lex.EncNoFlags)
		revokeQuery := fmt.Sprintf(`REVOKE ALL ON TABLE %s, %s FROM %s`,
			f.GetFQFileTableName(), f.GetFQPayloadTableName(), quotedUsername.String())

I think you don't need this quotedUsername buffer, you may be able to solve this with tree.Name(user).String()?

[adityamaru]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @knz)

---

pkg/cli/userfile.go, line 363 at r1 (raw file):

Previously, knz (kena) wrote…

1. make a TableName
2. add the suffix _upload_files to your tablename's .ObjectName
3. call String on the TableName to get the properly quoted result.

Done.

---

pkg/cli/userfiletable_test.go, line 511 at r1 (raw file):

Previously, knz (kena) wrote…

these are the rules for SQL-defined usernames but 1) we may change them tomorrow 2) we are soon going to accept username definitions from outside of SQL which may have different rules.

I think the test that would be nice to see is how the names are derived for identifiers that contain arbitrary characters, regardless of whether they are currently valid SQL users.

Also, I forgot an example:

family (or index)

these two usernames are valid according to current rules but they are also reserved keywords, and so the test might reveal some additional quoting is necessary.

Just to clarify, this fix is only to be backported to 20.2 so the set of valid users can't change from beneath us? In 21.1 we have a much more reliable scheme that we had worked on earlier in this release #56046.
Nonetheless, I have added some testing that verifies how we create table names in these cases! Also added index.

---

pkg/storage/cloudimpl/filetable/file_table_read_writer.go, line 832 at r2 (raw file):

Previously, knz (kena) wrote…

I think you don't need this quotedUsername buffer, you may be able to solve this with tree.Name(user).String()?

ah nice, tree.NameString does the appropriate quoting.

[knz]

Reviewed 3 of 3 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt)

---

pkg/cli/userfiletable_test.go, line 511 at r1 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

Just to clarify, this fix is only to be backported to 20.2 so the set of valid users can't change from beneath us? In 21.1 we have a much more reliable scheme that we had worked on earlier in this release #56046.
Nonetheless, I have added some testing that verifies how we create table names in these cases! Also added index.

cool that works for me.

[adityamaru]

TFTR!