From 1d47359d3a05c6ed69264c129314b45a1499707d Mon Sep 17 00:00:00 2001 From: BornChanger Date: Wed, 18 Jan 2023 23:33:41 +0800 Subject: [PATCH 1/9] ddl: raise error when drop resource group some user depends on Signed-off-by: BornChanger --- ddl/ddl_api.go | 14 ++++++++++++++ ddl/resource_group_test.go | 5 +++++ 2 files changed, 19 insertions(+) diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index c2a319f30b059..702fd3313ae7f 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -7668,6 +7668,20 @@ func (d *ddl) DropResourceGroup(ctx sessionctx.Context, stmt *ast.DropResourceGr return err } + // check to see if some user has dependency on the group + exec := ctx.(sqlexec.RestrictedSQLExecutor) + internal_ctx := kv.WithInternalSourceType(context.Background(), kv.InternalTxnDDL) + sql := "select count(*) from mysql.user where lower(json_extract(user_attributes, '$.resource_group')) ='\"" + groupName.L + "\"'" + rows, _, err := exec.ExecRestrictedSQL(internal_ctx, nil, sql) + if err != nil { + err = errors.Errorf("system table mysql.user access failed") + return err + } + if len(rows) != 0 && rows[0].GetInt64(0) > 0 { + err = errors.Errorf("some user depends on the resource group to drop") + return err + } + job := &model.Job{ SchemaID: group.ID, SchemaName: group.Name.L, diff --git a/ddl/resource_group_test.go b/ddl/resource_group_test.go index 0693247b25ce1..cb40a82e59650 100644 --- a/ddl/resource_group_test.go +++ b/ddl/resource_group_test.go @@ -160,6 +160,11 @@ func TestResourceGroupBasic(t *testing.T) { tk.MustGetErrCode("create user usr_fail resource group nil_group", mysql.ErrResourceGroupNotExists) tk.MustExec("create user user2") tk.MustGetErrCode("alter user user2 resource group nil_group", mysql.ErrResourceGroupNotExists) + + tk.MustExec("create resource group do_not_delete_rg rru_per_sec=100 wru_per_sec=200") + tk.MustExec("create user usr3 resource group do_not_delete_rg") + //tk.MustQuery("select json_extract(user_attributes, '$.resource_group') from mysql.user where user='usr3'").Check(testkit.Rows("1")) + tk.MustContainErrMsg("drop resource group do_not_delete_rg", "some user depends on the resource group to drop") } func testResourceGroupNameFromIS(t *testing.T, ctx sessionctx.Context, name string) *model.ResourceGroupInfo { From 213d4e14c49c8ac2767700988ad89f35bea67060 Mon Sep 17 00:00:00 2001 From: BornChanger Date: Wed, 18 Jan 2023 23:46:26 +0800 Subject: [PATCH 2/9] ddl: cleanup case Signed-off-by: BornChanger --- ddl/resource_group_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/ddl/resource_group_test.go b/ddl/resource_group_test.go index cb40a82e59650..cd2d0188994c5 100644 --- a/ddl/resource_group_test.go +++ b/ddl/resource_group_test.go @@ -163,7 +163,6 @@ func TestResourceGroupBasic(t *testing.T) { tk.MustExec("create resource group do_not_delete_rg rru_per_sec=100 wru_per_sec=200") tk.MustExec("create user usr3 resource group do_not_delete_rg") - //tk.MustQuery("select json_extract(user_attributes, '$.resource_group') from mysql.user where user='usr3'").Check(testkit.Rows("1")) tk.MustContainErrMsg("drop resource group do_not_delete_rg", "some user depends on the resource group to drop") } From 58ce01718cdf0453b6628b0085d7f9b3e2ebee23 Mon Sep 17 00:00:00 2001 From: BornChanger Date: Wed, 18 Jan 2023 23:51:43 +0800 Subject: [PATCH 3/9] ddl: code fmt Signed-off-by: BornChanger --- ddl/ddl_api.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index 702fd3313ae7f..7189342b044f5 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -7670,9 +7670,9 @@ func (d *ddl) DropResourceGroup(ctx sessionctx.Context, stmt *ast.DropResourceGr // check to see if some user has dependency on the group exec := ctx.(sqlexec.RestrictedSQLExecutor) - internal_ctx := kv.WithInternalSourceType(context.Background(), kv.InternalTxnDDL) + internalCtx := kv.WithInternalSourceType(context.Background(), kv.InternalTxnDDL) sql := "select count(*) from mysql.user where lower(json_extract(user_attributes, '$.resource_group')) ='\"" + groupName.L + "\"'" - rows, _, err := exec.ExecRestrictedSQL(internal_ctx, nil, sql) + rows, _, err := exec.ExecRestrictedSQL(internalCtx, nil, sql) if err != nil { err = errors.Errorf("system table mysql.user access failed") return err From 70cb86c61bb0b9c35e271f00ed56cfe22d7bd12e Mon Sep 17 00:00:00 2001 From: BornChanger Date: Thu, 19 Jan 2023 11:52:36 +0800 Subject: [PATCH 4/9] *: refind error message Signed-off-by: BornChanger --- ddl/ddl_api.go | 6 +++--- ddl/resource_group_test.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index 7189342b044f5..59248c00b2a6d 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -7671,14 +7671,14 @@ func (d *ddl) DropResourceGroup(ctx sessionctx.Context, stmt *ast.DropResourceGr // check to see if some user has dependency on the group exec := ctx.(sqlexec.RestrictedSQLExecutor) internalCtx := kv.WithInternalSourceType(context.Background(), kv.InternalTxnDDL) - sql := "select count(*) from mysql.user where lower(json_extract(user_attributes, '$.resource_group')) ='\"" + groupName.L + "\"'" + sql := "select user from mysql.user where lower(json_extract(user_attributes, '$.resource_group')) ='\"" + groupName.L + "\"' limit 1" rows, _, err := exec.ExecRestrictedSQL(internalCtx, nil, sql) if err != nil { err = errors.Errorf("system table mysql.user access failed") return err } - if len(rows) != 0 && rows[0].GetInt64(0) > 0 { - err = errors.Errorf("some user depends on the resource group to drop") + if len(rows) != 0 { + err = errors.Errorf("user [%s] depends on the resource group to drop", rows[0].GetString(0)) return err } diff --git a/ddl/resource_group_test.go b/ddl/resource_group_test.go index cd2d0188994c5..789e81f99f0fb 100644 --- a/ddl/resource_group_test.go +++ b/ddl/resource_group_test.go @@ -163,7 +163,7 @@ func TestResourceGroupBasic(t *testing.T) { tk.MustExec("create resource group do_not_delete_rg rru_per_sec=100 wru_per_sec=200") tk.MustExec("create user usr3 resource group do_not_delete_rg") - tk.MustContainErrMsg("drop resource group do_not_delete_rg", "some user depends on the resource group to drop") + tk.MustContainErrMsg("drop resource group do_not_delete_rg", "user [usr3] depends on the resource group to drop") } func testResourceGroupNameFromIS(t *testing.T, ctx sessionctx.Context, name string) *model.ResourceGroupInfo { From e07f7dadbab05d90191c4ef84c9a7291d38f173b Mon Sep 17 00:00:00 2001 From: BornChanger Date: Thu, 19 Jan 2023 15:03:56 +0800 Subject: [PATCH 5/9] *: refine error message again Signed-off-by: BornChanger --- errno/errname.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/errno/errname.go b/errno/errname.go index 1c14f4517e247..2009563a96bec 100644 --- a/errno/errname.go +++ b/errno/errname.go @@ -1102,7 +1102,7 @@ var MySQLErrName = map[uint16]*mysql.ErrMessage{ ErrResourceGroupNotExists: mysql.Message("Unknown resource group '%-.192s'", nil), ErrColumnInChange: mysql.Message("column %s id %d does not exist, this column may have been updated by other DDL ran in parallel", nil), - ErrResourceGroupSupportDisabled: mysql.Message("Resource group feature is disabled", nil), + ErrResourceGroupSupportDisabled: mysql.Message("Resource control feature is disabled. Run `SET GLOBAL tidb_enable_resource_control='on'` to enable the feature", nil), // TiKV/PD errors. ErrPDServerTimeout: mysql.Message("PD server timeout: %s", nil), From 7902edee6c773f96a090e67425463439114efa2e Mon Sep 17 00:00:00 2001 From: BornChanger Date: Thu, 19 Jan 2023 15:50:09 +0800 Subject: [PATCH 6/9] *: add missing errors.toml Signed-off-by: BornChanger --- errors.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/errors.toml b/errors.toml index e2708289f132c..d9d1f76765a58 100644 --- a/errors.toml +++ b/errors.toml @@ -2533,7 +2533,7 @@ Unknown resource group '%-.192s' ["schema:8250"] error = ''' -Resource group feature is disabled +Resource control feature is disabled. Run `SET GLOBAL tidb_enable_resource_control='on'` to enable the feature ''' ["session:8002"] From fe9964ad7a5f1d9cccc63cc78431abb6a213be97 Mon Sep 17 00:00:00 2001 From: BornChanger Date: Thu, 19 Jan 2023 17:19:15 +0800 Subject: [PATCH 7/9] *: avoid using internal sql to check Signed-off-by: BornChanger --- ddl/ddl_api.go | 16 +++++++--------- privilege/privilege.go | 3 +++ privilege/privileges/cache.go | 11 +++++++++++ privilege/privileges/privileges.go | 10 ++++++++++ 4 files changed, 31 insertions(+), 9 deletions(-) diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index 59248c00b2a6d..73845400d85a5 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -22,6 +22,7 @@ import ( "bytes" "context" "fmt" + "github.com/pingcap/tidb/privilege" "math" "strconv" "strings" @@ -7669,16 +7670,13 @@ func (d *ddl) DropResourceGroup(ctx sessionctx.Context, stmt *ast.DropResourceGr } // check to see if some user has dependency on the group - exec := ctx.(sqlexec.RestrictedSQLExecutor) - internalCtx := kv.WithInternalSourceType(context.Background(), kv.InternalTxnDDL) - sql := "select user from mysql.user where lower(json_extract(user_attributes, '$.resource_group')) ='\"" + groupName.L + "\"' limit 1" - rows, _, err := exec.ExecRestrictedSQL(internalCtx, nil, sql) - if err != nil { - err = errors.Errorf("system table mysql.user access failed") - return err + checker := privilege.GetPrivilegeManager(ctx) + if checker == nil { + return errors.New("miss privilege checker") } - if len(rows) != 0 { - err = errors.Errorf("user [%s] depends on the resource group to drop", rows[0].GetString(0)) + user, matched := checker.MatchUserResourceGroupName(groupName.L) + if matched { + err = errors.Errorf("user [%s] depends on the resource group to drop", user) return err } diff --git a/privilege/privilege.go b/privilege/privilege.go index 47a6303ad2212..3229c1e3bb26f 100644 --- a/privilege/privilege.go +++ b/privilege/privilege.go @@ -87,6 +87,9 @@ type Manager interface { // MatchIdentity matches an identity MatchIdentity(user, host string, skipNameResolve bool) (string, string, bool) + // MatchUserResourceGroupName matches a user with specified resource group name + MatchUserResourceGroupName(resourceGroupName string) (string, bool) + // DBIsVisible returns true is the database is visible to current user. DBIsVisible(activeRole []*auth.RoleIdentity, db string) bool diff --git a/privilege/privileges/cache.go b/privilege/privileges/cache.go index caadb6e4f7397..9159777340b67 100644 --- a/privilege/privileges/cache.go +++ b/privilege/privileges/cache.go @@ -1014,6 +1014,17 @@ func (p *MySQLPrivilege) matchIdentity(user, host string, skipNameResolve bool) return nil } +// matchResoureGroup finds an identity to match resource group. +func (p *MySQLPrivilege) matchResoureGroup(resourceGroupName string) *UserRecord { + for i := 0; i < len(p.User); i++ { + record := &p.User[i] + if record.ResourceGroup == resourceGroupName { + return record + } + } + return nil +} + // connectionVerification verifies the username + hostname according to exact // match from the mysql.user privilege table. call matchIdentity() first if you // do not have an exact match yet. diff --git a/privilege/privileges/privileges.go b/privilege/privileges/privileges.go index 288415c7bcdb4..af038f984f413 100644 --- a/privilege/privileges/privileges.go +++ b/privilege/privileges/privileges.go @@ -310,6 +310,16 @@ func (p *UserPrivileges) MatchIdentity(user, host string, skipNameResolve bool) return "", "", false } +// MatchUserResourceGroupName implements the Manager interface. +func (p *UserPrivileges) MatchUserResourceGroupName(resourceGroupName string) (u string, success bool) { + mysqlPriv := p.Handle.Get() + record := mysqlPriv.matchResoureGroup(resourceGroupName) + if record != nil { + return record.User, true + } + return "", false +} + // GetAuthWithoutVerification implements the Manager interface. func (p *UserPrivileges) GetAuthWithoutVerification(user, host string) (success bool) { if SkipWithGrant { From 53ee2664f93d100af1a4f8ead99554dd14f3b22d Mon Sep 17 00:00:00 2001 From: BornChanger Date: Thu, 19 Jan 2023 17:32:45 +0800 Subject: [PATCH 8/9] *: code format Signed-off-by: BornChanger --- ddl/ddl_api.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index 73845400d85a5..3e256e5306fc8 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -22,7 +22,6 @@ import ( "bytes" "context" "fmt" - "github.com/pingcap/tidb/privilege" "math" "strconv" "strings" @@ -48,6 +47,7 @@ import ( "github.com/pingcap/tidb/parser/mysql" "github.com/pingcap/tidb/parser/terror" field_types "github.com/pingcap/tidb/parser/types" + "github.com/pingcap/tidb/privilege" "github.com/pingcap/tidb/sessionctx" "github.com/pingcap/tidb/sessionctx/variable" "github.com/pingcap/tidb/sessiontxn" From 2b7c5efda47d7f0b9343e2ee35fa94d3392eba73 Mon Sep 17 00:00:00 2001 From: BornChanger Date: Thu, 19 Jan 2023 18:23:07 +0800 Subject: [PATCH 9/9] *: fix bazel file Signed-off-by: BornChanger --- ddl/BUILD.bazel | 1 + 1 file changed, 1 insertion(+) diff --git a/ddl/BUILD.bazel b/ddl/BUILD.bazel index e9641c3e2a4c8..805e814924707 100644 --- a/ddl/BUILD.bazel +++ b/ddl/BUILD.bazel @@ -79,6 +79,7 @@ go_library( "//parser/opcode", "//parser/terror", "//parser/types", + "//privilege", "//sessionctx", "//sessionctx/binloginfo", "//sessionctx/stmtctx",