Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

*: Fix use of user identity in SHOW GRANTS + error messages #30294

Merged
merged 4 commits into from
Dec 7, 2021
Merged

*: Fix use of user identity in SHOW GRANTS + error messages #30294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c871648
*: Fix use of user identity in error messages
morgo Nov 30, 2021
8ae35ed
Merge branch 'master' into fix-show-grants-again
morgo Dec 1, 2021
83645fc
Merge branch 'master' into fix-show-grants-again
morgo Dec 2, 2021
688bb8a
Merge branch 'master' into fix-show-grants-again
ti-chi-bot Dec 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions expression/builtin_info.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ func (b *builtinCurrentUserSig) evalString(row chunk.Row) (string, bool, error)
if data == nil || data.User == nil {
return "", true, errors.Errorf("Missing session variable when eval builtin")
}
return data.User.AuthIdentityString(), false, nil
return data.User.String(), false, nil
}

type currentRoleFunctionClass struct {
Expand Down Expand Up @@ -278,8 +278,7 @@ func (b *builtinUserSig) evalString(row chunk.Row) (string, bool, error) {
if data == nil || data.User == nil {
return "", true, errors.Errorf("Missing session variable when eval builtin")
}

return data.User.String(), false, nil
return data.User.LoginString(), false, nil
}

type connectionIDFunctionClass struct {
Expand Down
4 changes: 2 additions & 2 deletions expression/builtin_info_vec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ func (b *builtinCurrentUserSig) vecEvalString(input *chunk.Chunk, result *chunk.
return errors.Errorf("Missing session variable when eval builtin")
}
for i := 0; i < n; i++ {
result.AppendString(data.User.AuthIdentityString())
result.AppendString(data.User.String())
}
return nil
}
Expand Down Expand Up @@ -168,7 +168,7 @@ func (b *builtinUserSig) vecEvalString(input *chunk.Chunk, result *chunk.Column)

result.ReserveString(n)
for i := 0; i < n; i++ {
result.AppendString(data.User.String())
result.AppendString(data.User.LoginString())
}
return nil
}
Expand Down
12 changes: 9 additions & 3 deletions parser/auth/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,18 +41,24 @@ func (user *UserIdentity) Restore(ctx *format.RestoreCtx) error {
}

// String converts UserIdentity to the format user@host.
// It defaults to providing the AuthIdentity (the matching entry in priv tables)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: This totally change the semantic of the old API, and all the code referencing of the old API might be affected.

AuthIdentityString() -> String()
String() -> LoginString()

I can't say the new API is better or necessay (Go also discourage public API change), but as long as it fix more cases and do not introduce regression, it looks good to me.

// To use the actual identity use LoginString()
func (user *UserIdentity) String() string {
// TODO: Escape username and hostname.
if user == nil {
return ""
}
if user.AuthUsername != "" {
return fmt.Sprintf("%s@%s", user.AuthUsername, user.AuthHostname)
}
return fmt.Sprintf("%s@%s", user.Username, user.Hostname)
}

// AuthIdentityString returns matched identity in user@host format
func (user *UserIdentity) AuthIdentityString() string {
// LoginString returns matched identity in user@host format
// It matches the login user.
func (user *UserIdentity) LoginString() string {
// TODO: Escape username and hostname.
return fmt.Sprintf("%s@%s", user.AuthUsername, user.AuthHostname)
return fmt.Sprintf("%s@%s", user.Username, user.Hostname)
}

type RoleIdentity struct {
Expand Down
29 changes: 29 additions & 0 deletions privilege/privileges/privileges_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,35 @@ func TestShowGrants(t *testing.T) {
require.Len(t, gs, 3)
}

// TestErrorMessage checks that the identity in error messages matches the mysql.user table one.
// MySQL is inconsistent in its error messages, as some match the loginHost and others the
// identity from mysql.user. In TiDB we now use the identity from mysql.user in error messages
// for consistency.
func TestErrorMessage(t *testing.T) {
Copy link
Contributor

@dveeden dveeden Dec 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manually testing this with v5.3.0 and master @ 13d0deb show the same error messages as in this test. Are you sure this test fails without your fix?

[dvaneeden@dve-carbon ~]$ mysqlsh mysql://[email protected]:4000

sql> SHOW CREATE USER 'specifichost'@'192.168.1.54'\G
*************************** 1. row ***************************
CREATE USER for [email protected]: CREATE USER 'specifichost'@'192.168.1.54' IDENTIFIED WITH 'mysql_native_password' AS '' REQUIRE NONE PASSWORD EXPIRE DEFAULT ACCOUNT UNLOCK
1 row in set (0.0015 sec)

sql> SHOW CREATE USER 'wildcard'@'%'\G
*************************** 1. row ***************************
CREATE USER for wildcard@%: CREATE USER 'wildcard'@'%' IDENTIFIED WITH 'mysql_native_password' AS '' REQUIRE NONE PASSWORD EXPIRE DEFAULT ACCOUNT UNLOCK
1 row in set (0.0022 sec)

sql> 
Bye!
[dvaneeden@dve-carbon ~]$ mysqlsh mysql://[email protected]:4000
Please provide the password for '[email protected]:4000': 
Save password for '[email protected]:4000'? [Y]es/[N]o/Ne[v]er (default No): 

sql> USE mysql
MySQL Error 1044: Access denied for user 'wildcard'@'%' to database 'mysql'

sql> 
Bye!
[dvaneeden@dve-carbon ~]$ mysqlsh mysql://[email protected]:4000
Please provide the password for '[email protected]:4000': 
Save password for '[email protected]:4000'? [Y]es/[N]o/Ne[v]er (default No): 

sql> USE mysql
MySQL Error 1044: Access denied for user 'specifichost'@'192.168.1.54' to database 'mysql'

sql> SELECT tidb_version()\G
*************************** 1. row ***************************
tidb_version(): Release Version: v5.4.0-alpha-274-g13d0deb7f
Edition: Community
Git Commit Hash: 13d0deb7fef7fd7391dce19f320abef4256223bd
Git Branch: master
UTC Build Time: 2021-12-01 08:28:22
GoVersion: go1.16.8
Race Enabled: false
TiKV Min Version: v3.0.0-60965b006877ca7234adaced7890d7b029ed1306
Check Table Before Drop: false
1 row in set (0.0005 sec)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this for future checking.

The case where it currently fails prior to this is the Column name in SHOW GRANTS FOR CURRENT_USER(). But in the tests I didn't find an example where we check column names.

t.Parallel()
store, clean := newStore(t)
defer clean()

rootSe := newSession(t, store, dbName)
mustExec(t, rootSe, `CREATE USER wildcard`)
mustExec(t, rootSe, `CREATE USER [email protected]`)
mustExec(t, rootSe, `GRANT SELECT on test.* TO wildcard`)
mustExec(t, rootSe, `GRANT SELECT on test.* TO [email protected]`)

wildSe := newSession(t, store, dbName)

// The session.Auth() func will populate the AuthUsername and AuthHostname fields.
// We don't have to explicitly specify them.
require.True(t, wildSe.Auth(&auth.UserIdentity{Username: "wildcard", Hostname: "192.168.1.1"}, nil, nil))
_, err := wildSe.ExecuteInternal(context.Background(), "use mysql;")
require.Equal(t, "[executor:1044]Access denied for user 'wildcard'@'%' to database 'mysql'", err.Error())

specificSe := newSession(t, store, dbName)
require.True(t, specificSe.Auth(&auth.UserIdentity{Username: "specifichost", Hostname: "192.168.1.1"}, nil, nil))
_, err = specificSe.ExecuteInternal(context.Background(), "use mysql;")
require.Equal(t, "[executor:1044]Access denied for user 'specifichost'@'192.168.1.1' to database 'mysql'", err.Error())
}

func TestShowColumnGrants(t *testing.T) {
t.Parallel()
store, clean := newStore(t)
Expand Down