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.

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

*: fix revoke statement for CURRENT_USER() and refine error message #24052

Merged
merged 28 commits into from
May 12, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
38e8549
*: fix revoke statement for CURRENT_USER() and refine error message
tiancaiamao Apr 15, 2021
4a963b2
fix errdoc check
tiancaiamao Apr 15, 2021
ef17dd2
fix CI and go mod tidy
tiancaiamao Apr 15, 2021
2eeb5ec
fix CI?
tiancaiamao Apr 15, 2021
a540edf
Merge branch 'master' into privilege
ti-chi-bot Apr 28, 2021
43c1e75
Merge branch 'master' into privilege
ti-chi-bot Apr 28, 2021
a426031
Merge branch 'master' into privilege
ti-chi-bot Apr 28, 2021
37b39f4
Merge branch 'master' into privilege
ti-chi-bot Apr 28, 2021
b100ea9
Merge branch 'master' into privilege
ti-chi-bot Apr 28, 2021
405deb7
Merge branch 'master' into privilege
ti-chi-bot Apr 28, 2021
d9d227d
Merge branch 'master' into privilege
ti-chi-bot Apr 28, 2021
b485c28
Merge branch 'master' into privilege
ti-chi-bot Apr 28, 2021
234fb43
Merge branch 'master' into privilege
ti-chi-bot Apr 28, 2021
4cd90ca
Merge branch 'master' into privilege
ti-chi-bot Apr 28, 2021
6f6dad8
Merge branch 'master' into privilege
ti-chi-bot Apr 28, 2021
71269b1
Merge branch 'master' into privilege
ti-chi-bot Apr 28, 2021
2b7e7a6
Merge branch 'master' into privilege
ti-chi-bot Apr 28, 2021
87cbe15
Merge branch 'master' into privilege
ti-chi-bot Apr 28, 2021
4673e92
Merge branch 'master' into privilege
ti-chi-bot Apr 28, 2021
e0b495b
Merge branch 'master' into privilege
ti-chi-bot Apr 28, 2021
a279c34
Merge branch 'master' into privilege
ti-chi-bot Apr 28, 2021
beffc2d
Merge branch 'master' into privilege
ti-chi-bot Apr 28, 2021
4487714
Merge branch 'master' into privilege
ti-chi-bot Apr 29, 2021
6009917
Merge branch 'master' into privilege
ti-chi-bot Apr 29, 2021
410676f
Merge branch 'master' into privilege
ti-chi-bot Apr 29, 2021
772180e
Merge branch 'master' into privilege
ti-chi-bot Apr 29, 2021
830b6d6
Merge branch 'master' into privilege
ti-chi-bot Apr 29, 2021
840f66e
Merge branch 'master' into privilege
ti-chi-bot May 11, 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
2 changes: 1 addition & 1 deletion errno/errname.go
Original file line number Diff line number Diff line change
Expand Up @@ -1020,7 +1020,7 @@ var MySQLErrName = map[uint16]*mysql.ErrMessage{
ErrBuildExecutor: mysql.Message("Failed to build executor", nil),
ErrBatchInsertFail: mysql.Message("Batch insert failed, please clean the table and try again.", nil),
ErrGetStartTS: mysql.Message("Can not get start ts", nil),
ErrPrivilegeCheckFail: mysql.Message("privilege check fail", nil), // this error message should begin lowercased to be compatible with the test
ErrPrivilegeCheckFail: mysql.Message("privilege check for '%s' fail", nil), // this error message should begin lowercased to be compatible with the test
ErrInvalidWildCard: mysql.Message("Wildcard fields without any table name appears in wrong place", nil),
ErrMixOfGroupFuncAndFieldsIncompatible: mysql.Message("In aggregated query without GROUP BY, expression #%d of SELECT list contains nonaggregated column '%s'; this is incompatible with sql_mode=only_full_group_by", nil),
ErrUnsupportedSecondArgumentType: mysql.Message("JSON_OBJECTAGG: unsupported second argument type %v", nil),
Expand Down
2 changes: 1 addition & 1 deletion errors.toml
Original file line number Diff line number Diff line change
Expand Up @@ -1133,7 +1133,7 @@ Schema has changed

["planner:8121"]
error = '''
privilege check fail
privilege check for '%s' fail
'''

["planner:8122"]
Expand Down
6 changes: 6 additions & 0 deletions executor/revoke.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,14 @@ func (e *RevokeExec) Next(ctx context.Context, req *chunk.Chunk) error {
return err
}

sessVars := e.ctx.GetSessionVars()
// Revoke for each user.
for _, user := range e.Users {
if user.User.CurrentUser {
user.User.Username = sessVars.User.AuthUsername
user.User.Hostname = sessVars.User.AuthHostname
}

// Check if user exists.
exists, err := userExists(e.ctx, user.User.Username, user.User.Hostname)
if err != nil {
Expand Down
4 changes: 2 additions & 2 deletions planner/core/optimizer.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,13 +103,13 @@ func CheckPrivilege(activeRoles []*auth.RoleIdentity, pm privilege.Manager, vs [
if v.privilege == mysql.ExtendedPriv {
if !pm.RequestDynamicVerification(activeRoles, v.dynamicPriv, v.dynamicWithGrant) {
if v.err == nil {
return ErrPrivilegeCheckFail
return ErrPrivilegeCheckFail.GenWithStackByArgs(v.dynamicPriv)
}
return v.err
}
} else if !pm.RequestVerification(activeRoles, v.db, v.table, v.column, v.privilege) {
if v.err == nil {
return ErrPrivilegeCheckFail
return ErrPrivilegeCheckFail.GenWithStackByArgs(v.privilege.String())
}
return v.err
}
Expand Down
2 changes: 1 addition & 1 deletion planner/core/point_get_plan.go
Original file line number Diff line number Diff line change
Expand Up @@ -994,7 +994,7 @@ func checkFastPlanPrivilege(ctx sessionctx.Context, dbName, tableName string, ch
var visitInfos []visitInfo
for _, checkType := range checkTypes {
if pm != nil && !pm.RequestVerification(ctx.GetSessionVars().ActiveRoles, dbName, tableName, "", checkType) {
return errors.New("privilege check fail")
return ErrPrivilegeCheckFail.GenWithStackByArgs(checkType.String())
}
// This visitInfo is only for table lock check, so we do not need column field,
// just fill it empty string.
Expand Down
16 changes: 11 additions & 5 deletions privilege/privileges/privileges_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,12 @@ func (s *testPrivilegeSuite) TestRevokePrivileges(c *C) {
c.Assert(se.Auth(&auth.UserIdentity{Username: "hasgrant", Hostname: "localhost", AuthUsername: "hasgrant", AuthHostname: "%"}, nil, nil), IsTrue)
mustExec(c, se, "REVOKE SELECT ON mysql.* FROM 'withoutgrant'")
mustExec(c, se, "REVOKE ALL ON mysql.* FROM withoutgrant")

// For issue https://github.com/pingcap/tidb/issues/23850
mustExec(c, se, "CREATE USER u4")
Copy link
Member

Choose a reason for hiding this comment

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

Could you add test cases for u1, u2, u3 as shown in #23850?

mustExec(c, se, "GRANT ALL ON *.* TO u4 WITH GRANT OPTION")
c.Assert(se.Auth(&auth.UserIdentity{Username: "u4", Hostname: "localhost", AuthUsername: "u4", AuthHostname: "%"}, nil, nil), IsTrue)
mustExec(c, se, "REVOKE ALL ON *.* FROM CURRENT_USER()")
}

func (s *testPrivilegeSuite) TestSetGlobal(c *C) {
Expand Down Expand Up @@ -1006,14 +1012,14 @@ func (s *testPrivilegeSuite) TestSystemSchema(c *C) {
_, err = se.ExecuteInternal(context.Background(), "drop table information_schema.tables")
c.Assert(strings.Contains(err.Error(), "denied to user"), IsTrue)
_, err = se.ExecuteInternal(context.Background(), "update information_schema.tables set table_name = 'tst' where table_name = 'mysql'")
c.Assert(strings.Contains(err.Error(), "privilege check fail"), IsTrue)
c.Assert(strings.Contains(err.Error(), "privilege check"), IsTrue)

// Test performance_schema.
mustExec(c, se, `select * from performance_schema.events_statements_summary_by_digest`)
_, err = se.ExecuteInternal(context.Background(), "drop table performance_schema.events_statements_summary_by_digest")
c.Assert(strings.Contains(err.Error(), "denied to user"), IsTrue)
_, err = se.ExecuteInternal(context.Background(), "update performance_schema.events_statements_summary_by_digest set schema_name = 'tst'")
c.Assert(strings.Contains(err.Error(), "privilege check fail"), IsTrue)
c.Assert(strings.Contains(err.Error(), "privilege check"), IsTrue)
_, err = se.ExecuteInternal(context.Background(), "delete from performance_schema.events_statements_summary_by_digest")
c.Assert(strings.Contains(err.Error(), "DELETE command denied to user"), IsTrue)
_, err = se.ExecuteInternal(context.Background(), "create table performance_schema.t(a int)")
Expand All @@ -1025,7 +1031,7 @@ func (s *testPrivilegeSuite) TestSystemSchema(c *C) {
_, err = se.ExecuteInternal(context.Background(), "drop table metrics_schema.tidb_query_duration")
c.Assert(strings.Contains(err.Error(), "denied to user"), IsTrue)
_, err = se.ExecuteInternal(context.Background(), "update metrics_schema.tidb_query_duration set instance = 'tst'")
c.Assert(strings.Contains(err.Error(), "privilege check fail"), IsTrue)
c.Assert(strings.Contains(err.Error(), "privilege check"), IsTrue)
_, err = se.ExecuteInternal(context.Background(), "delete from metrics_schema.tidb_query_duration")
c.Assert(strings.Contains(err.Error(), "DELETE command denied to user"), IsTrue)
_, err = se.ExecuteInternal(context.Background(), "create table metric_schema.t(a int)")
Expand All @@ -1041,9 +1047,9 @@ func (s *testPrivilegeSuite) TestAdminCommand(c *C) {

c.Assert(se.Auth(&auth.UserIdentity{Username: "test_admin", Hostname: "localhost"}, nil, nil), IsTrue)
_, err := se.ExecuteInternal(context.Background(), "ADMIN SHOW DDL JOBS")
c.Assert(strings.Contains(err.Error(), "privilege check fail"), IsTrue)
c.Assert(strings.Contains(err.Error(), "privilege check"), IsTrue)
_, err = se.ExecuteInternal(context.Background(), "ADMIN CHECK TABLE t")
c.Assert(strings.Contains(err.Error(), "privilege check fail"), IsTrue)
c.Assert(strings.Contains(err.Error(), "privilege check"), IsTrue)

c.Assert(se.Auth(&auth.UserIdentity{Username: "root", Hostname: "localhost"}, nil, nil), IsTrue)
_, err = se.ExecuteInternal(context.Background(), "ADMIN SHOW DDL JOBS")
Expand Down
2 changes: 1 addition & 1 deletion session/session_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2892,7 +2892,7 @@ func (s *testSessionSuite2) TestUpdatePrivilege(c *C) {

_, err := tk1.Exec("update t2 set id = 666 where id = 1;")
c.Assert(err, NotNil)
c.Assert(strings.Contains(err.Error(), "privilege check fail"), IsTrue)
c.Assert(strings.Contains(err.Error(), "privilege check"), IsTrue)

// Cover a bug that t1 and t2 both require update privilege.
// In fact, the privlege check for t1 should be update, and for t2 should be select.
Expand Down