planner: add more test cases for column privilege | tidb-test=pr/2704#66542
Conversation
|
@CbcWestwolf This PR has conflicts, I have hold it. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
@ti-chi-bot: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
winoros
left a comment
There was a problem hiding this comment.
don't forget to resolve conflicts
c92f198 to
c63474e
Compare
📝 WalkthroughWalkthroughThis pull request expands column-level privilege checking across the query planner and executor. Key changes include per-column UPDATE privilege validation for INSERT ON DUPLICATE KEY UPDATE operations, updated column name resolution in join coalescing, comprehensive test coverage for column-level privileges in various DML/DDL scenarios, and new privilege tests for REVOKE, TRACE, EXPLAIN, and IMPORT INTO operations. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Session
participant Planner as Query Planner
participant PrivilegeChecker as Privilege Checker
participant Executor as Executor
User->>Planner: INSERT ... ON DUPLICATE KEY UPDATE
Planner->>Planner: Parse INSERT statement
Planner->>PrivilegeChecker: Check INSERT privilege on table
PrivilegeChecker-->>Planner: ✓ Table privilege granted
Planner->>Planner: Resolve ON DUPLICATE columns
Planner->>PrivilegeChecker: Check UPDATE privilege per column
PrivilegeChecker-->>Planner: ✓ Column privileges validated
Planner->>Planner: Generate execution plan with visitInfo
Planner-->>Executor: Plan + per-column privilege metadata
Executor->>Executor: Execute INSERT or UPDATE as needed
Executor-->>User: Result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/planner/core/planbuilder.go (1)
4300-4326:⚠️ Potential issue | 🟠 MajorUse resolved target-table metadata for the UPDATE privilege entry.
This new
visitInfois derived fromp.tableColNames[idx], but those names come from the output-name list built withtn.Schema. For unqualifiedINSERT INTO t ... ON DUPLICATE KEY UPDATE ..., that can leave the DB name empty and record the UPDATE privilege at the wrong scope. Please pass the resolved DB name/table name intoresolveOnDuplicateand build the privilege entry from the target table metadata instead ofFieldName.Suggested fix
- onDupColSet, err := insertPlan.resolveOnDuplicate(insert.OnDuplicate, tableInfo, b, func(node ast.ExprNode) (expression.Expression, error) { + onDupColSet, err := insertPlan.resolveOnDuplicate(insert.OnDuplicate, tnW.DBInfo.Name.L, tableInfo, b, func(node ast.ExprNode) (expression.Expression, error) { return b.rewriteInsertOnDuplicateUpdate(ctx, node, mockTablePlan, insertPlan) })-func (p *Insert) resolveOnDuplicate(onDup []*ast.Assignment, tblInfo *model.TableInfo, b *PlanBuilder, yield func(ast.ExprNode) (expression.Expression, error)) (map[string]struct{}, error) { +func (p *Insert) resolveOnDuplicate(onDup []*ast.Assignment, dbName string, tblInfo *model.TableInfo, b *PlanBuilder, yield func(ast.ExprNode) (expression.Expression, error)) (map[string]struct{}, error) { onDupColSet := make(map[string]struct{}, len(onDup)) colMap := make(map[string]*table.Column, len(p.Table.Cols())) for _, col := range p.Table.Cols() { colMap[col.Name.L] = col } user := b.ctx.GetSessionVars().User for _, assign := range onDup { @@ - fieldName := p.tableColNames[idx] var authErr error if user != nil { - authErr = plannererrors.ErrColumnaccessDenied.FastGenByArgs("UPDATE", user.AuthUsername, user.AuthHostname, fieldName.OrigColName.L, fieldName.OrigTblName.L) + authErr = plannererrors.ErrColumnaccessDenied.FastGenByArgs("UPDATE", user.AuthUsername, user.AuthHostname, column.Name.L, tblInfo.Name.L) } - b.visitInfo = appendVisitInfo(b.visitInfo, mysql.UpdatePriv, fieldName.DBName.L, fieldName.OrigTblName.L, fieldName.OrigColName.L, authErr) + b.visitInfo = appendVisitInfo(b.visitInfo, mysql.UpdatePriv, dbName, tblInfo.Name.L, column.Name.L, authErr)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/planbuilder.go` around lines 4300 - 4326, The UPDATE privilege entry currently uses fieldName (from p.tableColNames) which can lack the resolved DB/table; instead use the resolved target table metadata (tblInfo) when building the privilege and auth error. In resolveOnDuplicate, replace uses of fieldName.DBName.L and fieldName.OrigTblName.L in the appendVisitInfo call (and the FastGenByArgs creation of authErr) with the resolved names from tblInfo (e.g., tblInfo.DBName.L and tblInfo.Name.L) while keeping the column name from fieldName.OrigColName.L so the UPDATE privilege is recorded at the correct DB/table scope.pkg/planner/core/logical_plans_test.go (2)
1876-1881:⚠️ Potential issue | 🟡 MinorError message column mismatch in test case.
Line 1879 specifies column
"d"in the visitInfo but the error message references"c"inFastGenByArgs. This appears to be a copy-paste error.Proposed fix
- {mysql.SelectPriv, "test", "t", "d", plannererrors.ErrColumnaccessDenied.FastGenByArgs("SELECT", "", "", "c", "t"), false, nil, false}, + {mysql.SelectPriv, "test", "t", "d", plannererrors.ErrColumnaccessDenied.FastGenByArgs("SELECT", "", "", "d", "t"), false, nil, false},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/logical_plans_test.go` around lines 1876 - 1881, The test has a copy-paste error in the visitInfo row for column "d": the expected error constructed by plannererrors.ErrColumnaccessDenied.FastGenByArgs uses "c" instead of "d", causing a mismatch; update the FastGenByArgs call in that entry so the column argument is "d" (i.e., make the FastGenByArgs call match the visitInfo column "d") to fix the assertion.
1923-1928:⚠️ Potential issue | 🟡 MinorError message column mismatch in test case.
Line 1926 has the same issue: the visitInfo specifies column
"d"but the error message references"c".Proposed fix
- {mysql.SelectPriv, "test", "t", "d", plannererrors.ErrColumnaccessDenied.FastGenByArgs("SELECT", "", "", "c", "t"), false, nil, false}, + {mysql.SelectPriv, "test", "t", "d", plannererrors.ErrColumnaccessDenied.FastGenByArgs("SELECT", "", "", "d", "t"), false, nil, false},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/logical_plans_test.go` around lines 1923 - 1928, The test data has a mismatch: the visitInfo (column "d") but the expected error uses "c"; update the fourth entry in the table so the plannererrors.ErrColumnaccessDenied.FastGenByArgs call uses "d" (i.e., FastGenByArgs("SELECT", "", "", "d", "t")) to match the visitInfo; locate the entries in the test slice in logical_plans_test.go (the rows with mysql.SelectPriv, "test", "t", ...) and fix only the error message argument for the row that currently repeats "c".
🧹 Nitpick comments (1)
tests/realtikvtest/importintotest2/from_select_test.go (1)
178-180: Assert the denied column name here, not just error 1143.This loop currently passes on any column-access failure. Since the regression is specifically about per-column privilege resolution, checking that the error mentions the current ungranted column (
a, thenb, thenc) would make the test much more diagnostic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/realtikvtest/importintotest2/from_select_test.go` around lines 178 - 180, The test currently only checks for mysql.ErrColumnaccessDenied in the loop (using userTk.MustGetErrCode(importSQL,...)) which doesn't verify which column caused the denial; change the call to capture the actual error (e.g. use a variant that returns error instead of MustGetErrCode) when running importSQL, then assert that err != nil, that errors.Is(err, mysql.ErrColumnaccessDenied) and that err.Error() contains the current colName (use fmt.Sprintf("%q", colName) or fmt.Sprintf("column %s", colName) as appropriate) so the loop verifies the denied column for each iteration (referencing importSQL, userTk.MustGetErrCode usage and grantSQL in the loop).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/realtikvtest/importintotest2/from_select_test.go`:
- Around line 162-183: This test leaks global state by creating/using the shared
"test" schema and a global user; update TestImportFromSelectPrivilege to operate
entirely in the suite-local schema returned by prepareAndUseDB("from_select")
instead of hard-coded "test" (replace "test.t" and "test.t1" with the local
schema-qualified names or use the test kit's current DB), create and grant the
user scoped to that local schema (or ensure the user is dropped in teardown),
and remove any global fixtures by adding explicit DROP USER and DROP TABLE
cleanup inside the test (or use defer teardown) so all resources created by
TestImportFromSelectPrivilege (functions: TestImportFromSelectPrivilege,
prepareAndUseDB, userTk.Session().Auth, and the GRANT/IMPORT statements) stay
local and are always cleaned up.
---
Outside diff comments:
In `@pkg/planner/core/logical_plans_test.go`:
- Around line 1876-1881: The test has a copy-paste error in the visitInfo row
for column "d": the expected error constructed by
plannererrors.ErrColumnaccessDenied.FastGenByArgs uses "c" instead of "d",
causing a mismatch; update the FastGenByArgs call in that entry so the column
argument is "d" (i.e., make the FastGenByArgs call match the visitInfo column
"d") to fix the assertion.
- Around line 1923-1928: The test data has a mismatch: the visitInfo (column
"d") but the expected error uses "c"; update the fourth entry in the table so
the plannererrors.ErrColumnaccessDenied.FastGenByArgs call uses "d" (i.e.,
FastGenByArgs("SELECT", "", "", "d", "t")) to match the visitInfo; locate the
entries in the test slice in logical_plans_test.go (the rows with
mysql.SelectPriv, "test", "t", ...) and fix only the error message argument for
the row that currently repeats "c".
In `@pkg/planner/core/planbuilder.go`:
- Around line 4300-4326: The UPDATE privilege entry currently uses fieldName
(from p.tableColNames) which can lack the resolved DB/table; instead use the
resolved target table metadata (tblInfo) when building the privilege and auth
error. In resolveOnDuplicate, replace uses of fieldName.DBName.L and
fieldName.OrigTblName.L in the appendVisitInfo call (and the FastGenByArgs
creation of authErr) with the resolved names from tblInfo (e.g.,
tblInfo.DBName.L and tblInfo.Name.L) while keeping the column name from
fieldName.OrigColName.L so the UPDATE privilege is recorded at the correct
DB/table scope.
---
Nitpick comments:
In `@tests/realtikvtest/importintotest2/from_select_test.go`:
- Around line 178-180: The test currently only checks for
mysql.ErrColumnaccessDenied in the loop (using
userTk.MustGetErrCode(importSQL,...)) which doesn't verify which column caused
the denial; change the call to capture the actual error (e.g. use a variant that
returns error instead of MustGetErrCode) when running importSQL, then assert
that err != nil, that errors.Is(err, mysql.ErrColumnaccessDenied) and that
err.Error() contains the current colName (use fmt.Sprintf("%q", colName) or
fmt.Sprintf("column %s", colName) as appropriate) so the loop verifies the
denied column for each iteration (referencing importSQL, userTk.MustGetErrCode
usage and grantSQL in the loop).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 09e79847-c41e-4d2a-9006-6e6e2d2fbb0a
📒 Files selected for processing (9)
pkg/executor/revoke_test.gopkg/planner/core/logical_plan_builder.gopkg/planner/core/logical_plans_test.gopkg/planner/core/planbuilder.gopkg/privilege/privileges/privileges_test.gotests/integrationtest/r/privilege/privileges.resulttests/integrationtest/t/privilege/privileges.testtests/realtikvtest/importintotest2/BUILD.bazeltests/realtikvtest/importintotest2/from_select_test.go
| func (s *mockGCSSuite) TestImportFromSelectPrivilege() { | ||
| s.prepareAndUseDB("from_select") | ||
| s.tk.MustExec("DROP USER IF EXISTS 'testuser'@'localhost';") | ||
| s.tk.MustExec("DROP TABLE IF EXISTS test.t") | ||
| s.tk.MustExec("DROP TABLE IF EXISTS test.t1") | ||
| s.tk.MustExec(`CREATE USER 'testuser'@'localhost';`) | ||
| s.tk.MustExec(`CREATE TABLE test.t (a INT, b INT, c INT);`) | ||
| s.tk.MustExec(`CREATE TABLE test.t1 (a INT, b INT, c INT);`) | ||
| userTk := testkit.NewTestKit(s.T(), s.store) | ||
| err := userTk.Session().Auth(&auth.UserIdentity{Username: "testuser", Hostname: "localhost"}, nil, nil, nil) | ||
| require.NoError(s.T(), err) | ||
| s.tk.MustExec("GRANT ALL ON test.t1 TO 'testuser'@'localhost'") | ||
|
|
||
| importSQL := "IMPORT INTO test.t1 FROM SELECT a,b,c FROM test.t" | ||
| grantSQL := "GRANT SELECT(%s) ON test.t TO 'testuser'@'localhost'" | ||
|
|
||
| for _, colName := range []string{"a", "b", "c"} { | ||
| userTk.MustGetErrCode(importSQL, mysql.ErrColumnaccessDenied) | ||
| s.tk.MustExec(fmt.Sprintf(grantSQL, colName)) | ||
| } | ||
| userTk.MustExec(importSQL) | ||
| } |
There was a problem hiding this comment.
Keep this case inside the suite-local schema and clean up its global fixtures.
prepareAndUseDB("from_select") gives this test an isolated schema, but the body switches to test.t / test.t1 and creates a global user without deferred teardown. That makes the case depend on the shared test database and can leak state into later RealTiKV cases if it fails partway through.
Suggested fix
func (s *mockGCSSuite) TestImportFromSelectPrivilege() {
s.prepareAndUseDB("from_select")
- s.tk.MustExec("DROP USER IF EXISTS 'testuser'@'localhost';")
- s.tk.MustExec("DROP TABLE IF EXISTS test.t")
- s.tk.MustExec("DROP TABLE IF EXISTS test.t1")
+ const dbName = "from_select"
+ s.tk.MustExec("DROP USER IF EXISTS 'testuser'@'localhost'")
+ defer s.tk.MustExec("DROP USER IF EXISTS 'testuser'@'localhost'")
s.tk.MustExec(`CREATE USER 'testuser'@'localhost';`)
- s.tk.MustExec(`CREATE TABLE test.t (a INT, b INT, c INT);`)
- s.tk.MustExec(`CREATE TABLE test.t1 (a INT, b INT, c INT);`)
+ s.tk.MustExec("DROP TABLE IF EXISTS " + dbName + ".t")
+ s.tk.MustExec("DROP TABLE IF EXISTS " + dbName + ".t1")
+ defer s.tk.MustExec("DROP TABLE IF EXISTS " + dbName + ".t1")
+ defer s.tk.MustExec("DROP TABLE IF EXISTS " + dbName + ".t")
+ s.tk.MustExec(`CREATE TABLE ` + dbName + `.t (a INT, b INT, c INT);`)
+ s.tk.MustExec(`CREATE TABLE ` + dbName + `.t1 (a INT, b INT, c INT);`)
userTk := testkit.NewTestKit(s.T(), s.store)
err := userTk.Session().Auth(&auth.UserIdentity{Username: "testuser", Hostname: "localhost"}, nil, nil, nil)
require.NoError(s.T(), err)
- s.tk.MustExec("GRANT ALL ON test.t1 TO 'testuser'@'localhost'")
+ s.tk.MustExec("GRANT ALL ON " + dbName + ".t1 TO 'testuser'@'localhost'")
- importSQL := "IMPORT INTO test.t1 FROM SELECT a,b,c FROM test.t"
- grantSQL := "GRANT SELECT(%s) ON test.t TO 'testuser'@'localhost'"
+ importSQL := "IMPORT INTO " + dbName + ".t1 FROM SELECT a,b,c FROM " + dbName + ".t"
+ grantSQL := "GRANT SELECT(%s) ON " + dbName + ".t TO 'testuser'@'localhost'"As per coding guidelines, tests/realtikvtest/**: RealTiKV tests MUST start playground in background, run tests, then clean up playground/data, and **/{*_test.go,testdata/**,*.golden} changes should stay minimal and deterministic.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *mockGCSSuite) TestImportFromSelectPrivilege() { | |
| s.prepareAndUseDB("from_select") | |
| s.tk.MustExec("DROP USER IF EXISTS 'testuser'@'localhost';") | |
| s.tk.MustExec("DROP TABLE IF EXISTS test.t") | |
| s.tk.MustExec("DROP TABLE IF EXISTS test.t1") | |
| s.tk.MustExec(`CREATE USER 'testuser'@'localhost';`) | |
| s.tk.MustExec(`CREATE TABLE test.t (a INT, b INT, c INT);`) | |
| s.tk.MustExec(`CREATE TABLE test.t1 (a INT, b INT, c INT);`) | |
| userTk := testkit.NewTestKit(s.T(), s.store) | |
| err := userTk.Session().Auth(&auth.UserIdentity{Username: "testuser", Hostname: "localhost"}, nil, nil, nil) | |
| require.NoError(s.T(), err) | |
| s.tk.MustExec("GRANT ALL ON test.t1 TO 'testuser'@'localhost'") | |
| importSQL := "IMPORT INTO test.t1 FROM SELECT a,b,c FROM test.t" | |
| grantSQL := "GRANT SELECT(%s) ON test.t TO 'testuser'@'localhost'" | |
| for _, colName := range []string{"a", "b", "c"} { | |
| userTk.MustGetErrCode(importSQL, mysql.ErrColumnaccessDenied) | |
| s.tk.MustExec(fmt.Sprintf(grantSQL, colName)) | |
| } | |
| userTk.MustExec(importSQL) | |
| } | |
| func (s *mockGCSSuite) TestImportFromSelectPrivilege() { | |
| s.prepareAndUseDB("from_select") | |
| const dbName = "from_select" | |
| s.tk.MustExec("DROP USER IF EXISTS 'testuser'@'localhost'") | |
| defer s.tk.MustExec("DROP USER IF EXISTS 'testuser'@'localhost'") | |
| s.tk.MustExec(`CREATE USER 'testuser'@'localhost';`) | |
| s.tk.MustExec("DROP TABLE IF EXISTS " + dbName + ".t") | |
| s.tk.MustExec("DROP TABLE IF EXISTS " + dbName + ".t1") | |
| defer s.tk.MustExec("DROP TABLE IF EXISTS " + dbName + ".t1") | |
| defer s.tk.MustExec("DROP TABLE IF EXISTS " + dbName + ".t") | |
| s.tk.MustExec(`CREATE TABLE ` + dbName + `.t (a INT, b INT, c INT);`) | |
| s.tk.MustExec(`CREATE TABLE ` + dbName + `.t1 (a INT, b INT, c INT);`) | |
| userTk := testkit.NewTestKit(s.T(), s.store) | |
| err := userTk.Session().Auth(&auth.UserIdentity{Username: "testuser", Hostname: "localhost"}, nil, nil, nil) | |
| require.NoError(s.T(), err) | |
| s.tk.MustExec("GRANT ALL ON " + dbName + ".t1 TO 'testuser'@'localhost'") | |
| importSQL := "IMPORT INTO " + dbName + ".t1 FROM SELECT a,b,c FROM " + dbName + ".t" | |
| grantSQL := "GRANT SELECT(%s) ON " + dbName + ".t TO 'testuser'@'localhost'" | |
| for _, colName := range []string{"a", "b", "c"} { | |
| userTk.MustGetErrCode(importSQL, mysql.ErrColumnaccessDenied) | |
| s.tk.MustExec(fmt.Sprintf(grantSQL, colName)) | |
| } | |
| userTk.MustExec(importSQL) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/realtikvtest/importintotest2/from_select_test.go` around lines 162 -
183, This test leaks global state by creating/using the shared "test" schema and
a global user; update TestImportFromSelectPrivilege to operate entirely in the
suite-local schema returned by prepareAndUseDB("from_select") instead of
hard-coded "test" (replace "test.t" and "test.t1" with the local
schema-qualified names or use the test kit's current DB), create and grant the
user scoped to that local schema (or ensure the user is dropped in teardown),
and remove any global fixtures by adding explicit DROP USER and DROP TABLE
cleanup inside the test (or use defer teardown) so all resources created by
TestImportFromSelectPrivilege (functions: TestImportFromSelectPrivilege,
prepareAndUseDB, userTk.Session().Auth, and the GRANT/IMPORT statements) stay
local and are always cleaned up.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #66542 +/- ##
================================================
Coverage ? 55.0066%
================================================
Files ? 1819
Lines ? 653276
Branches ? 0
================================================
Hits ? 359345
Misses ? 267259
Partials ? 26672
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fzzf678, winoros The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
/retest |
This is an automated cherry-pick of #62241
What problem does this PR solve?
Issue Number: ref #61706
Problem Summary:
What changed and how does it work?
JOINUSINGorNATURAL JOIN, plan builder will advance the order of joined columns, so the column withcommonLenshould be checkedON DUPLICATE KEY UPDATEUPDATEprivilege are those in theSET xxx =, not those needINSERTprivilege. See the test case: https://github.com/pingcap/tidb/pull/62241/files#diff-822cc6dca28387214a64c2ced297843e1cb8ac8d4c86246b139a8e79494dd220R1582-R1590Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests