auth: added quoting for account names on output.

mjonss commented 3 years ago

What problem does this PR solve?

While working on RENAME USER, I noticed that the output for errors related to accounts was not the same as for MySQL, so I added the missing quoting for TiDB.

What is changed and how it works?

Only quoting the name and host for accounts so user@host becomes 'user'@'host'

Check List

Tests

Unit test Done, including verified that auth_test.go is actually tested.
Manual test, verified that all new code is covered.

Code changes

Has exported function/method change - New exported function EscapeAccountName(string) string

Side effects

Changes/corrects error output.

Related changes

N/A

kennytm commented 3 years ago

I noted that AuthIdentityString() is also used in the CURRENT_USER() SQL function. Does this PR affect the output? (Do we want to be compatible with MySQL here?)

create user `u'v"w\x@y``z`;
-- ...
select current_user();
+----------------+
| current_user() |
+----------------+
| u'v"w\x@y`z@%  |
+----------------+
1 row in set (0.00 sec)

mjonss commented 3 years ago

I noted that AuthIdentityString() is also used in the CURRENT_USER() SQL function. Does this PR affect the output? (Do we want to be compatible with MySQL here?)

Good catch, I think it makes sense to be keep being compatible with MySQL here. I will try to address this as well.

tiancaiamao commented 3 years ago

\' means ', so the name \' should not be interpreted as \\'' ? @mjonss

mysql> select '\'';
+---+
| ' |
+---+
| ' |
+---+
1 row in set (0.00 sec)

mysql> select '\\''';
+----+
| \' |
+----+
| \' |
+----+
1 row in set (0.00 sec)

mjonss commented 3 years ago

\' means ', so the name \' should not be interpreted as \\'' ? @mjonss

In which context do you mean? Do you refer to any test in this PR? Or is it a test that is missing?

The strings.ReplaceAll(s, "'", "''") will replace the raw string (which should not be affected by the escape character, but only convert the raw string to a single quote string. ' as the string of a single quote char will be returned as the quoted string of four single quote chars '''' Depending on quoting and language/system used when specifying the string, the escape character \ will be interpreted differently: Quoted string -> raw string "\'" (not allowed in go, compile time error, but for mysql) -> ' "\\'" -> \' '\'' (not allowed in go, runtime error, but for mysql) -> ' `\'` -> \' '''' (not allowed in go, compile time error, but for mysql) -> ' '''''' (not allowed in go, compile time error, but for mysql) -> ''

But as noticed above too, is that the current_user() function also uses AuthIdentityString(), but should not be quoted, so the patch to change the String() functions in this repo (parser) together with the parser repo cannot adjust to the sql_mode NO_BACKSLASH_ESCAPES (see above), I think I will close this PR unless objections within a week, since it is better to implement it in the tidb repository directly instead I think. Which also makes it easier to keep the output in sync between the different repositories.

tiancaiamao commented 3 years ago

I doubt strings.ReplaceAll(s, "'", "''") could do the correct job.

The raw name a user intend is \', literally, combined a \ with a '

Then it has to be written as \\\' because of the mysql escaping:

mysql> select '\\\'';
+----+
| \' |
+----+
| \' |
+----+
1 row in set (0.00 sec)

In Go, strings.ReplaceAll() turns \\\' into '\\\'''

And this is what the user intend any more. @mjonss

mjonss commented 3 years ago

I doubt strings.ReplaceAll(s, "'", "''") could do the correct job. ...

The first problem is to define the 'correct job', which is already inconsistent in MySQL and gets even more complicated with the sql_mode = "NO_BACKSLASH_ESCAPES"

Some examples of the inconsistencies:

mysql> set sql_mode = "";
Query OK, 0 rows affected (0,00 sec)

mysql> create user "\\'"@testname; -- raw string (\')
Query OK, 0 rows affected (0,02 sec)

mysql> create user "\\'"@testname; -- raw string (\') reported with backslash escaped string in error
ERROR 1396 (HY000): Operation CREATE USER failed for '\\\''@'testname'
mysql> set sql_mode = "NO_BACKSLASH_ESCAPES";
Query OK, 0 rows affected (0,00 sec)

mysql> create user "\'"@testname; -- raw string (\') reported with duplicated single quote.
ERROR 1396 (HY000): Operation CREATE USER failed for '\'''@'testname'
mysql> create user '\'''@testname; -- raw string (\') due to NO_BACKSLASH_ESCAPE sql_mode
ERROR 1396 (HY000): Operation CREATE USER failed for '\'''@'testname'
mysql> set sql_mode = "";
Query OK, 0 rows affected (0,00 sec)

-- not possible without NO_BACKSLASH_ESCAPE
mysql> create user '\'''@testname;
    '> '\c
mysql> select user,host from mysql.user where host = 'testname'; -- will return the raw string
+------+----------+
| user | host     |
+------+----------+
| \'   | testname |
+------+----------+
1 row in set (0,00 sec)

mysql> create table quote_test (a varchar(255) not null primary key);
Query OK, 0 rows affected (0,08 sec)

mysql> set sql_mode = "";
Query OK, 0 rows affected (0,00 sec)

-- not possible without sql_mode NO_BACKSLASH_ESCAPES
mysql> insert into quote_test values ('\''');
    '> \c
    '> '\c
mysql> insert into quote_test values ("\\'"); -- raw string (\')
Query OK, 1 row affected (0,04 sec)

mysql> insert into quote_test values ('\\\''); -- raw string (\') badly escaped in error message
ERROR 1062 (23000): Duplicate entry '\'' for key 'quote_test.PRIMARY'
mysql> set sql_mode = "NO_BACKSLASH_ESCAPES";
Query OK, 0 rows affected (0,00 sec)

mysql> insert into quote_test values ('\'''); -- raw string (\') badly escaped in error message
ERROR 1062 (23000): Duplicate entry '\'' for key 'quote_test.PRIMARY'
mysql>

I don't think we should support the inconsistencies as MySQL above, but implement the correct and consistent behaviour. To do that we need to be aware of the sql_mode NO_BACKSLASH_ESCAPES. And I don't see any good way in handling this in the parser repository (unless it just implements a helper function with an argument on how to handle backslashes).

So that is why I proposed to close this PR (unless objections) and create a new issue in tidb repository with a more clear definition of what to implement, which in my view should work like MySQL when it comes to results, but be correct and consistent for warnings and errors. One way to say it is correct is that the output should also be valid input in the same context/sql_mode.

tiancaiamao commented 3 years ago

There is a SetSQLMode function https://github.com/pingcap/parser/blob/master/yy_parser.go#L188 and that's how ANSI_SQL mode handled. https://github.com/pingcap/parser/blob/1599cceb78b94ca3f273d165a12ec8988927d595/lexer.go#L170

It seems NO_BACKSLASH_ESCAPES is not considered yet.

mjonss commented 3 years ago

I will close this for now, and if needed we can restart the conversation and how to address this, since it depends on sql_mode

pingcap / parser