privileges,planner: implement checks for RESTRICTED_USER_ADMIN for granting privileges and roles (#64297)#66647
Open
ti-chi-bot wants to merge 1 commit intopingcap:release-8.1from
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
13 tasks
|
This cherry pick PR is for a release branch and has not yet been approved by triage owners. To merge this cherry pick:
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. |
Member
Author
|
@YangKeao 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThis PR extends privilege enforcement to validate RESTRICTED_USER_ADMIN constraints during grant and revoke operations. The planbuilder now appends restricted-user visitor checks after processing GrantStmt, GrantRoleStmt, RevokeStmt, and RevokeRoleStmt. The appendDynamicVisitInfo function signature is updated to accept privilege slices instead of single strings, and comprehensive tests validate the enhanced privilege semantics across multiple SEM versions. Changes
Sequence DiagramsequenceDiagram
participant Client
participant PlanBuilder
participant PrivilegeVisitor
participant PrivilegeCheck as Privilege Verifier
Client->>PlanBuilder: Grant/Revoke Statement
PlanBuilder->>PlanBuilder: buildSimple()
PlanBuilder->>PrivilegeCheck: Existing privilege checks
PrivilegeCheck-->>PlanBuilder: Status
PlanBuilder->>PrivilegeVisitor: appendVisitInfoIsRestrictedUser()
PrivilegeVisitor->>PrivilegeVisitor: Validate RESTRICTED_USER_ADMIN
PrivilegeVisitor-->>PlanBuilder: Restricted user checks
PlanBuilder-->>Client: Execution plan
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
|
@ti-chi-bot: The following tests 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. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
pkg/privilege/privileges/privileges_test.go (2)
2156-2194: Guard global var mutation with cleanup for test isolation.
TestSQLVariableAccelerateUserCreationUpdatemutates@@global.tidb_accelerate_user_creation_updatebut only restores it on the happy path. Addt.Cleanup(...)to prevent cross-test leakage on early failures.♻️ Suggested adjustment
func TestSQLVariableAccelerateUserCreationUpdate(t *testing.T) { store := createStoreAndPrepareDB(t) tk := testkit.NewTestKit(t, store) + t.Cleanup(func() { + tk.MustExec("set @@global.tidb_accelerate_user_creation_update = off") + }) dom := domain.GetDomain(tk.Session())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/privilege/privileges/privileges_test.go` around lines 2156 - 2194, TestSQLVariableAccelerateUserCreationUpdate mutates @@global.tidb_accelerate_user_creation_update without guaranteeing restore on failures; add a t.Cleanup that captures the original value (query via tk.MustQuery or read vardef.AccelerateUserCreationUpdate.Load()) before calling tk.MustExec("set @@global.tidb_accelerate_user_creation_update = ...") and restore it in the cleanup using tk.MustExec to set it back, ensuring cleanup is registered immediately after creating tk so the global var is reset even if the test fails early.
2273-2282: Add explicitRENAME USERcoverage for restricted users.This block validates ALTER USER (password change), but the objective also mentions blocking name modification. Please add a direct
RENAME USERassertion pair (denied fornormal_user, allowed forrestricted_user) to lock regression coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/privilege/privileges/privileges_test.go` around lines 2273 - 2282, Add explicit RENAME USER assertions beside the existing ALTER USER checks: for tk3 (authenticated as normal_user) call ExecToErr("RENAME USER restricted_user_2 TO restricted_user_2_renamed") and require.EqualError(...) using the same expected error string "[planner:1227]Access denied; you need (at least one of) the RESTRICTED_USER_ADMIN privilege(s) for this operation"; for tk4 (authenticated as restricted_user) call ExecToErr("RENAME USER restricted_user_2 TO restricted_user_2_renamed") and require.NoError(...) to assert the rename succeeds. Use the same testkit instances tk3 and tk4 and the same target user name restricted_user_2 to mirror the ALTER USER coverage.pkg/planner/core/planbuilder.go (1)
3365-3367: Remove duplicate REVOKE restricted-user checksThis loop is redundant:
collectVisitInfoFromRevokeStmtalready appends the same restricted-user checks (Lines 3471-3474). Keeping both duplicatesvisitInfoentries unnecessarily.💡 Proposed cleanup
- for _, user := range raw.Users { - b.visitInfo = appendVisitInfoIsRestrictedUser(ctx, b.visitInfo, b.ctx, user.User, "RESTRICTED_USER_ADMIN") - }🤖 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 3365 - 3367, Remove the redundant loop that iterates over raw.Users and calls appendVisitInfoIsRestrictedUser (which appends "RESTRICTED_USER_ADMIN") because collectVisitInfoFromRevokeStmt already performs the same append; delete that duplicate iteration (the loop referencing raw.Users and b.visitInfo) and keep the single append in collectVisitInfoFromRevokeStmt to avoid duplicate visitInfo entries while preserving use of appendVisitInfoIsRestrictedUser and visitInfo.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/planner/core/planbuilder.go`:
- Around line 3312-3314: The calls to appendVisitInfoIsRestrictedUser are using
an extra ctx argument and don't match the helper signature (visitInfo, sctx,
user, priv); update each call (lines where you see
appendVisitInfoIsRestrictedUser) to pass exactly four args and assign back to
b.visitInfo, e.g. replace the current call with b.visitInfo =
appendVisitInfoIsRestrictedUser(b.visitInfo, b.ctx, user.User,
"RESTRICTED_USER_ADMIN") (apply the same change for the other occurrences at the
noted spots).
- Around line 3337-3346: There is an unresolved merge conflict in the
GrantRoleStmt block and several incorrect calls to
appendVisitInfoIsRestrictedUser; remove the conflict markers and keep the
intended code that appends ROLE_ADMIN via appendDynamicVisitInfo(b.visitInfo,
[]string{"ROLE_ADMIN"}, false, err) then iterate raw.Roles to call
appendVisitInfoIsRestrictedUser with the correct signature and argument order
(appendVisitInfoIsRestrictedUser(b.visitInfo, b.ctx,
&auth.UserIdentity{Username: role.Username, Hostname: role.Hostname},
"RESTRICTED_USER_ADMIN")) and also add the RESTRICTED_ROLE_ADMIN check where
needed (e.g., when any role is RESTRICTED require RESTRICTED_ROLE_ADMIN); update
all other mis-invocations (GrantStmt, RevokeRoleStmt, RevokeStmt) to the correct
form appendVisitInfoIsRestrictedUser(b.visitInfo, b.ctx, user,
"RESTRICTED_USER_ADMIN") and run the planner unit tests (go test -run <TestName>
-tags=intest,deadlock) to validate.
In `@pkg/privilege/privileges/privileges_test.go`:
- Around line 2119-2319: The file still contains unresolved merge markers
(<<<<<<<, =======, >>>>>>>) around the added tests (e.g.,
TestEnsureActiveUserCoverage and TestSQLVariableAccelerateUserCreationUpdate);
remove the conflict markers and keep the intended merged blocks (all new test
functions and the final TestProtectUserAndRoleWithRestrictedPrivileges calls).
Also add the missing imports for domain.GetDomain and
vardef.AccelerateUserCreationUpdate by importing
github.com/pingcap/tidb/pkg/domain and
github.com/pingcap/tidb/pkg/sessionctx/vardef in the test file's import block so
the symbols in TestSQLVariableAccelerateUserCreationUpdate compile.
---
Nitpick comments:
In `@pkg/planner/core/planbuilder.go`:
- Around line 3365-3367: Remove the redundant loop that iterates over raw.Users
and calls appendVisitInfoIsRestrictedUser (which appends
"RESTRICTED_USER_ADMIN") because collectVisitInfoFromRevokeStmt already performs
the same append; delete that duplicate iteration (the loop referencing raw.Users
and b.visitInfo) and keep the single append in collectVisitInfoFromRevokeStmt to
avoid duplicate visitInfo entries while preserving use of
appendVisitInfoIsRestrictedUser and visitInfo.
In `@pkg/privilege/privileges/privileges_test.go`:
- Around line 2156-2194: TestSQLVariableAccelerateUserCreationUpdate mutates
@@global.tidb_accelerate_user_creation_update without guaranteeing restore on
failures; add a t.Cleanup that captures the original value (query via
tk.MustQuery or read vardef.AccelerateUserCreationUpdate.Load()) before calling
tk.MustExec("set @@global.tidb_accelerate_user_creation_update = ...") and
restore it in the cleanup using tk.MustExec to set it back, ensuring cleanup is
registered immediately after creating tk so the global var is reset even if the
test fails early.
- Around line 2273-2282: Add explicit RENAME USER assertions beside the existing
ALTER USER checks: for tk3 (authenticated as normal_user) call ExecToErr("RENAME
USER restricted_user_2 TO restricted_user_2_renamed") and
require.EqualError(...) using the same expected error string
"[planner:1227]Access denied; you need (at least one of) the
RESTRICTED_USER_ADMIN privilege(s) for this operation"; for tk4 (authenticated
as restricted_user) call ExecToErr("RENAME USER restricted_user_2 TO
restricted_user_2_renamed") and require.NoError(...) to assert the rename
succeeds. Use the same testkit instances tk3 and tk4 and the same target user
name restricted_user_2 to mirror the ALTER USER coverage.
Comment on lines
+3312
to
+3314
| for _, user := range raw.Users { | ||
| b.visitInfo = appendVisitInfoIsRestrictedUser(ctx, b.visitInfo, b.ctx, user.User, "RESTRICTED_USER_ADMIN") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify helper signature.
echo "=== Function signature ==="
rg -n 'func appendVisitInfoIsRestrictedUser\(' pkg/planner/core/planbuilder.go -C2
echo ""
echo "=== All call sites ==="
# Verify all call sites and inspect arity/order manually.
rg -n 'appendVisitInfoIsRestrictedUser\(' pkg/planner/core/planbuilder.go -C2Repository: pingcap/tidb
Length of output: 3203
Fix appendVisitInfoIsRestrictedUser call signature at lines 3313, 3343, 3356, and 3366
The calls at these lines pass an extra ctx argument that doesn't match the function signature. The helper takes 4 args: (visitInfo, sctx, user, priv). These will fail to compile.
Proposed fix
for _, user := range raw.Users {
- b.visitInfo = appendVisitInfoIsRestrictedUser(ctx, b.visitInfo, b.ctx, user.User, "RESTRICTED_USER_ADMIN")
+ b.visitInfo = appendVisitInfoIsRestrictedUser(b.visitInfo, b.ctx, user.User, "RESTRICTED_USER_ADMIN")
}
for _, role := range raw.Roles {
- b.visitInfo = appendVisitInfoIsRestrictedUser(ctx, b.visitInfo, b.ctx, &auth.UserIdentity{Username: role.Username, Hostname: role.Hostname},
+ b.visitInfo = appendVisitInfoIsRestrictedUser(b.visitInfo, b.ctx, &auth.UserIdentity{Username: role.Username, Hostname: role.Hostname},
"RESTRICTED_USER_ADMIN")
}
for _, user := range raw.Users {
- b.visitInfo = appendVisitInfoIsRestrictedUser(ctx, b.visitInfo, b.ctx, user.User, "RESTRICTED_USER_ADMIN")
+ b.visitInfo = appendVisitInfoIsRestrictedUser(b.visitInfo, b.ctx, user.User, "RESTRICTED_USER_ADMIN")
}📝 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.
Suggested change
| for _, user := range raw.Users { | |
| b.visitInfo = appendVisitInfoIsRestrictedUser(ctx, b.visitInfo, b.ctx, user.User, "RESTRICTED_USER_ADMIN") | |
| } | |
| for _, user := range raw.Users { | |
| b.visitInfo = appendVisitInfoIsRestrictedUser(b.visitInfo, b.ctx, user.User, "RESTRICTED_USER_ADMIN") | |
| } |
🤖 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 3312 - 3314, The calls to
appendVisitInfoIsRestrictedUser are using an extra ctx argument and don't match
the helper signature (visitInfo, sctx, user, priv); update each call (lines
where you see appendVisitInfoIsRestrictedUser) to pass exactly four args and
assign back to b.visitInfo, e.g. replace the current call with b.visitInfo =
appendVisitInfoIsRestrictedUser(b.visitInfo, b.ctx, user.User,
"RESTRICTED_USER_ADMIN") (apply the same change for the other occurrences at the
noted spots).
Comment on lines
+3337
to
+3346
| <<<<<<< HEAD | ||
| b.visitInfo = appendDynamicVisitInfo(b.visitInfo, "ROLE_ADMIN", false, err) | ||
| ======= | ||
| b.visitInfo = appendDynamicVisitInfo(b.visitInfo, []string{"ROLE_ADMIN"}, false, err) | ||
| // If any of the roles are RESTRICTED, require RESTRICTED_ROLE_ADMIN | ||
| for _, role := range raw.Roles { | ||
| b.visitInfo = appendVisitInfoIsRestrictedUser(ctx, b.visitInfo, b.ctx, &auth.UserIdentity{Username: role.Username, Hostname: role.Hostname}, | ||
| "RESTRICTED_USER_ADMIN") | ||
| } | ||
| >>>>>>> 3328284abcb (privileges,planner: implement checks for `RESTRICTED_USER_ADMIN` for granting privileges and roles (#64297)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for merge conflict markers in the entire file
echo "=== Checking for merge conflict markers ==="
rg -n '^(<<<<<<<|=======|>>>>>>>)' pkg/planner/core/planbuilder.go || echo "No merge markers found"
# Read the specific region around lines 3337-3346 to see current state
echo -e "\n=== Current state of lines 3330-3350 ==="
sed -n '3330,3350p' pkg/planner/core/planbuilder.go | cat -n
# Find the GrantRoleStmt case to understand context
echo -e "\n=== GrantRoleStmt case context (lines 3335-3380) ==="
sed -n '3335,3380p' pkg/planner/core/planbuilder.go | cat -nRepository: pingcap/tidb
Length of output: 4671
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the appendVisitInfoIsRestrictedUser function definition
echo "=== Finding appendVisitInfoIsRestrictedUser function ==="
rg -n 'func.*appendVisitInfoIsRestrictedUser' pkg/planner/core/planbuilder.go -A 3
# Also search for all calls to appendVisitInfoIsRestrictedUser to see argument patterns
echo -e "\n=== All calls to appendVisitInfoIsRestrictedUser ==="
rg -n 'appendVisitInfoIsRestrictedUser' pkg/planner/core/planbuilder.go -B 1 -A 1Repository: pingcap/tidb
Length of output: 2391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there are planner tests related to GRANT/REVOKE or privileges
echo "=== Searching for planner tests related to GRANT/REVOKE statements ==="
find . -path '*/planner/**/*_test.go' -type f | head -20
# Look for test files that might test GrantStmt, RevokeStmt, or GrantRoleStmt
echo -e "\n=== Searching test files for GRANT/REVOKE related tests ==="
rg -l 'GrantStmt|RevokeStmt|GrantRoleStmt' --type go -g '**/*_test.go' | head -10Repository: pingcap/tidb
Length of output: 1059
Resolve merge-conflict markers at lines 3337-3346 and fix function signature mismatches throughout the block
The file contains an unresolved merge conflict at lines 3337-3346 in the GrantRoleStmt case (marked with <<<<<<< / ======= / >>>>>>>), which makes the file syntactically invalid and prevents compilation.
Additionally, multiple calls to appendVisitInfoIsRestrictedUser have incorrect signatures across the privilege-checking block:
- Correct signature:
appendVisitInfoIsRestrictedUser(visitInfo []visitInfo, sctx PlanContext, user *auth.UserIdentity, priv string)— 4 arguments - Incorrect calls (5 arguments with misaligned order):
- Line 3313 (GrantStmt):
appendVisitInfoIsRestrictedUser(ctx, b.visitInfo, b.ctx, user.User, "RESTRICTED_USER_ADMIN") - Line 3343 (in merge conflict):
appendVisitInfoIsRestrictedUser(ctx, b.visitInfo, b.ctx, &auth.UserIdentity{...}, "RESTRICTED_USER_ADMIN") - Line 3356 (RevokeRoleStmt):
appendVisitInfoIsRestrictedUser(ctx, b.visitInfo, b.ctx, &auth.UserIdentity{...}, "RESTRICTED_USER_ADMIN") - Line 3366 (RevokeStmt):
appendVisitInfoIsRestrictedUser(ctx, b.visitInfo, b.ctx, user.User, "RESTRICTED_USER_ADMIN")
- Line 3313 (GrantStmt):
Correct calls (e.g., lines 3352, 3379, 3395, 3399) use: appendVisitInfoIsRestrictedUser(b.visitInfo, b.ctx, user, priv).
Remove the ctx argument and reorder to match the function signature. Additionally, per the coding guidelines for pkg/planner/**, run targeted planner unit tests (go test -run <TestName> -tags=intest,deadlock) to validate these privilege-checking changes.
Merge conflict resolution example (GrantRoleStmt)
case *ast.GrantRoleStmt:
err := plannererrors.ErrSpecificAccessDenied.GenWithStackByArgs("SUPER or ROLE_ADMIN")
-<<<<<<< HEAD
- b.visitInfo = appendDynamicVisitInfo(b.visitInfo, "ROLE_ADMIN", false, err)
-=======
- b.visitInfo = appendDynamicVisitInfo(b.visitInfo, []string{"ROLE_ADMIN"}, false, err)
- // If any of the roles are RESTRICTED, require RESTRICTED_ROLE_ADMIN
- for _, role := range raw.Roles {
- b.visitInfo = appendVisitInfoIsRestrictedUser(ctx, b.visitInfo, b.ctx, &auth.UserIdentity{Username: role.Username, Hostname: role.Hostname},
- "RESTRICTED_USER_ADMIN")
- }
->>>>>>> 3328284abcb (privileges,planner: implement checks for `RESTRICTED_USER_ADMIN` for granting privileges and roles (`#64297`))
+ b.visitInfo = appendDynamicVisitInfo(b.visitInfo, []string{"ROLE_ADMIN"}, false, err)
+ for _, user := range raw.Users {
+ b.visitInfo = appendVisitInfoIsRestrictedUser(b.visitInfo, b.ctx, user, "RESTRICTED_USER_ADMIN")
+ }
+ for _, role := range raw.Roles {
+ b.visitInfo = appendVisitInfoIsRestrictedUser(
+ b.visitInfo,
+ b.ctx,
+ &auth.UserIdentity{Username: role.Username, Hostname: role.Hostname},
+ "RESTRICTED_USER_ADMIN",
+ )
+ }📝 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.
Suggested change
| <<<<<<< HEAD | |
| b.visitInfo = appendDynamicVisitInfo(b.visitInfo, "ROLE_ADMIN", false, err) | |
| ======= | |
| b.visitInfo = appendDynamicVisitInfo(b.visitInfo, []string{"ROLE_ADMIN"}, false, err) | |
| // If any of the roles are RESTRICTED, require RESTRICTED_ROLE_ADMIN | |
| for _, role := range raw.Roles { | |
| b.visitInfo = appendVisitInfoIsRestrictedUser(ctx, b.visitInfo, b.ctx, &auth.UserIdentity{Username: role.Username, Hostname: role.Hostname}, | |
| "RESTRICTED_USER_ADMIN") | |
| } | |
| >>>>>>> 3328284abcb (privileges,planner: implement checks for `RESTRICTED_USER_ADMIN` for granting privileges and roles (#64297)) | |
| b.visitInfo = appendDynamicVisitInfo(b.visitInfo, []string{"ROLE_ADMIN"}, false, err) | |
| for _, user := range raw.Users { | |
| b.visitInfo = appendVisitInfoIsRestrictedUser(b.visitInfo, b.ctx, user, "RESTRICTED_USER_ADMIN") | |
| } | |
| for _, role := range raw.Roles { | |
| b.visitInfo = appendVisitInfoIsRestrictedUser( | |
| b.visitInfo, | |
| b.ctx, | |
| &auth.UserIdentity{Username: role.Username, Hostname: role.Hostname}, | |
| "RESTRICTED_USER_ADMIN", | |
| ) | |
| } |
🤖 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 3337 - 3346, There is an
unresolved merge conflict in the GrantRoleStmt block and several incorrect calls
to appendVisitInfoIsRestrictedUser; remove the conflict markers and keep the
intended code that appends ROLE_ADMIN via appendDynamicVisitInfo(b.visitInfo,
[]string{"ROLE_ADMIN"}, false, err) then iterate raw.Roles to call
appendVisitInfoIsRestrictedUser with the correct signature and argument order
(appendVisitInfoIsRestrictedUser(b.visitInfo, b.ctx,
&auth.UserIdentity{Username: role.Username, Hostname: role.Hostname},
"RESTRICTED_USER_ADMIN")) and also add the RESTRICTED_ROLE_ADMIN check where
needed (e.g., when any role is RESTRICTED require RESTRICTED_ROLE_ADMIN); update
all other mis-invocations (GrantStmt, RevokeRoleStmt, RevokeStmt) to the correct
form appendVisitInfoIsRestrictedUser(b.visitInfo, b.ctx, user,
"RESTRICTED_USER_ADMIN") and run the planner unit tests (go test -run <TestName>
-tags=intest,deadlock) to validate.
Comment on lines
+2119
to
+2319
| <<<<<<< HEAD | ||
| ======= | ||
|
|
||
| func TestEnsureActiveUserCoverage(t *testing.T) { | ||
| store := createStoreAndPrepareDB(t) | ||
| tk := testkit.NewTestKit(t, store) | ||
| tk.MustExec("create user 'test'") | ||
| tk.Session().Auth(&auth.UserIdentity{Username: "root", Hostname: "localhost"}, nil, nil, nil) | ||
|
|
||
| cases := []struct { | ||
| sql string | ||
| visited bool | ||
| }{ | ||
| {"drop user if exists 'test1'", false}, | ||
| {"alter user test identified by 'test1'", false}, | ||
| {"set password for test = 'test2'", false}, | ||
| {"show create user test", false}, | ||
| {"create user test1", false}, | ||
| {"grant select on test.* to test1", false}, | ||
| {"show grants", true}, | ||
| {"show grants for 'test'@'%'", true}, | ||
| } | ||
|
|
||
| for ith, c := range cases { | ||
| var visited bool | ||
| ctx := context.WithValue(context.Background(), "mock", &visited) | ||
| rs, err := tk.ExecWithContext(ctx, c.sql) | ||
| require.NoError(t, err) | ||
|
|
||
| comment := fmt.Sprintf("testcase %d failed", ith) | ||
| if rs != nil { | ||
| tk.ResultSetToResultWithCtx(ctx, rs, comment) | ||
| } | ||
| require.Equal(t, c.visited, visited, comment) | ||
| } | ||
| } | ||
|
|
||
| func TestSQLVariableAccelerateUserCreationUpdate(t *testing.T) { | ||
| store := createStoreAndPrepareDB(t) | ||
| tk := testkit.NewTestKit(t, store) | ||
| dom := domain.GetDomain(tk.Session()) | ||
| // 1. check the default variable value | ||
| tk.MustQuery("select @@global.tidb_accelerate_user_creation_update").Check(testkit.Rows("0")) | ||
| // trigger priv reload | ||
| tk.MustExec("create user aaa") | ||
| handle := dom.PrivilegeHandle() | ||
| handle.CheckFullData(t, true) | ||
| priv := handle.Get() | ||
| require.False(t, priv.RequestVerification(nil, "bbb", "%", "test", "", "", mysql.SelectPriv)) | ||
|
|
||
| // 2. change the variable and check | ||
| tk.MustExec("set @@global.tidb_accelerate_user_creation_update = on") | ||
| tk.MustQuery("select @@global.tidb_accelerate_user_creation_update").Check(testkit.Rows("1")) | ||
| require.True(t, vardef.AccelerateUserCreationUpdate.Load()) | ||
| tk.MustExec("create user bbb") | ||
| handle.CheckFullData(t, false) | ||
| // trigger priv reload, but data for bbb is not really loaded | ||
| tk.MustExec("grant select on test.* to bbb") | ||
| priv = handle.Get() | ||
| // data for bbb is not loaded, because that user is not active | ||
| // So this is **counterintuitive**, but it's still the expected behavior. | ||
| require.False(t, priv.RequestVerification(nil, "bbb", "%", "test", "", "", mysql.SelectPriv)) | ||
| tk1 := testkit.NewTestKit(t, store) | ||
| // if user bbb login, everything works as expected | ||
| require.NoError(t, tk1.Session().Auth(&auth.UserIdentity{Username: "bbb", Hostname: "localhost"}, nil, nil, nil)) | ||
| priv = handle.Get() | ||
| require.True(t, priv.RequestVerification(nil, "bbb", "%", "test", "", "", mysql.SelectPriv)) | ||
|
|
||
| // 3. change the variable and check again | ||
| tk.MustExec("set @@global.tidb_accelerate_user_creation_update = off") | ||
| tk.MustQuery("select @@global.tidb_accelerate_user_creation_update").Check(testkit.Rows("0")) | ||
| tk.MustExec("drop user aaa") | ||
| handle.CheckFullData(t, true) | ||
| priv = handle.Get() | ||
| require.True(t, priv.RequestVerification(nil, "bbb", "%", "test", "", "", mysql.SelectPriv)) | ||
| } | ||
|
|
||
| func TestGrantOptionWithSEMv2(t *testing.T) { | ||
| store := createStoreAndPrepareDB(t) | ||
|
|
||
| rootTk := testkit.NewTestKit(t, store) | ||
| rootTk.MustExec("CREATE USER varuser1") | ||
| rootTk.MustExec("CREATE USER varuser2") | ||
| rootTk.MustExec("CREATE USER varuser3") | ||
| rootTk.MustExec("CREATE USER varuser4") | ||
| rootTk.MustExec("CREATE USER grantee") | ||
|
|
||
| rootTk.MustExec("GRANT SYSTEM_VARIABLES_ADMIN, FILE ON *.* TO varuser1") | ||
| rootTk.MustExec("GRANT SYSTEM_VARIABLES_ADMIN, FILE ON *.* TO varuser2 WITH GRANT OPTION") | ||
| rootTk.MustExec("GRANT RESTRICTED_PRIV_ADMIN ON *.* TO varuser3") | ||
| rootTk.MustExec("GRANT RESTRICTED_PRIV_ADMIN ON *.* TO varuser4") | ||
| rootTk.MustExec("GRANT SYSTEM_VARIABLES_ADMIN, FILE ON *.* TO varuser4 WITH GRANT OPTION") | ||
|
|
||
| // SYSTEM_VARIABLES_ADMIN is not restricted, FILE is restricted. | ||
| defer sem.SwitchToSEMForTest(t, sem.V2)() | ||
| // try to grant SYSTEM_VARIABLES_ADMIN and FILE privilege to grantee with different user | ||
| tk1 := testkit.NewTestKit(t, store) | ||
| require.NoError(t, tk1.Session().Auth(&auth.UserIdentity{Username: "varuser1", Hostname: "%"}, nil, nil, nil)) | ||
| err := tk1.ExecToErr("GRANT SYSTEM_VARIABLES_ADMIN ON *.* TO grantee") | ||
| require.EqualError(t, err, "[planner:1227]Access denied; you need (at least one of) the GRANT OPTION privilege(s) for this operation") | ||
| err = tk1.ExecToErr("GRANT FILE ON *.* TO grantee") | ||
| require.EqualError(t, err, "[planner:1227]Access denied; you need (at least one of) the RESTRICTED_PRIV_ADMIN privilege(s) for this operation") | ||
|
|
||
| tk2 := testkit.NewTestKit(t, store) | ||
| require.NoError(t, tk2.Session().Auth(&auth.UserIdentity{Username: "varuser2", Hostname: "%"}, nil, nil, nil)) | ||
| err = tk2.ExecToErr("GRANT SYSTEM_VARIABLES_ADMIN ON *.* TO grantee") | ||
| require.NoError(t, err) | ||
| err = tk2.ExecToErr("GRANT FILE ON *.* TO grantee") | ||
| require.EqualError(t, err, "[planner:1227]Access denied; you need (at least one of) the RESTRICTED_PRIV_ADMIN privilege(s) for this operation") | ||
|
|
||
| tk3 := testkit.NewTestKit(t, store) | ||
| require.NoError(t, tk3.Session().Auth(&auth.UserIdentity{Username: "varuser3", Hostname: "%"}, nil, nil, nil)) | ||
| err = tk3.ExecToErr("GRANT SYSTEM_VARIABLES_ADMIN ON *.* TO grantee") | ||
| require.EqualError(t, err, "[planner:1227]Access denied; you need (at least one of) the GRANT OPTION privilege(s) for this operation") | ||
| err = tk3.ExecToErr("GRANT FILE ON *.* TO grantee") | ||
| require.EqualError(t, err, "[planner:8121]privilege check for 'FILE' fail") | ||
|
|
||
| tk4 := testkit.NewTestKit(t, store) | ||
| require.NoError(t, tk4.Session().Auth(&auth.UserIdentity{Username: "varuser4", Hostname: "%"}, nil, nil, nil)) | ||
| err = tk4.ExecToErr("GRANT SYSTEM_VARIABLES_ADMIN ON *.* TO grantee") | ||
| require.NoError(t, err) | ||
| err = tk4.ExecToErr("GRANT FILE ON *.* TO grantee") | ||
| require.NoError(t, err) | ||
| } | ||
|
|
||
| func testProtectUserAndRoleWithRestrictedPrivileges(t *testing.T, semVer string) { | ||
| store := createStoreAndPrepareDB(t) | ||
|
|
||
| rootTk := testkit.NewTestKit(t, store) | ||
| rootTk.MustExec("CREATE USER restricted_user") | ||
| rootTk.MustExec("GRANT RESTRICTED_USER_ADMIN, SUPER, CREATE USER, SELECT ON *.* TO restricted_user WITH GRANT OPTION") | ||
| rootTk.MustExec("CREATE USER normal_user") | ||
| rootTk.MustExec("GRANT SUPER, CREATE USER, SELECT ON *.* TO normal_user WITH GRANT OPTION") | ||
|
|
||
| rootTk.MustExec("CREATE USER restricted_user_1") | ||
| rootTk.MustExec("GRANT RESTRICTED_USER_ADMIN ON *.* TO restricted_user_1") | ||
| rootTk.MustExec("CREATE USER restricted_user_2") | ||
| rootTk.MustExec("GRANT RESTRICTED_USER_ADMIN ON *.* TO restricted_user_2") | ||
| rootTk.MustExec("CREATE USER restricted_user_3") | ||
| rootTk.MustExec("GRANT RESTRICTED_USER_ADMIN ON *.* TO restricted_user_3") | ||
| rootTk.MustExec("CREATE USER normal_user_1") | ||
|
|
||
| defer sem.SwitchToSEMForTest(t, semVer)() | ||
| // Accounts with RESTRICTED_USER_ADMIN are not allowed to be deleted unless we also have the | ||
| // RESTRICTED_USER_ADMIN privilege | ||
| tk1 := testkit.NewTestKit(t, store) | ||
| require.NoError(t, tk1.Session().Auth(&auth.UserIdentity{Username: "normal_user", Hostname: "%"}, nil, nil, nil)) | ||
| err := tk1.ExecToErr("DROP USER restricted_user_1") | ||
| require.EqualError(t, err, "[planner:1227]Access denied; you need (at least one of) the RESTRICTED_USER_ADMIN privilege(s) for this operation") | ||
| tk2 := testkit.NewTestKit(t, store) | ||
| require.NoError(t, tk2.Session().Auth(&auth.UserIdentity{Username: "restricted_user", Hostname: "%"}, nil, nil, nil)) | ||
| err = tk2.ExecToErr("DROP USER restricted_user_1") | ||
| require.NoError(t, err) | ||
|
|
||
| // Accounts with RESTRICTED_USER_ADMIN are not allowed to be altered unless we also have the | ||
| // RESTRICTED_USER_ADMIN privilege | ||
| tk3 := testkit.NewTestKit(t, store) | ||
| require.NoError(t, tk3.Session().Auth(&auth.UserIdentity{Username: "normal_user", Hostname: "%"}, nil, nil, nil)) | ||
| err = tk3.ExecToErr("ALTER USER restricted_user_2 IDENTIFIED BY 'new_password'") | ||
| require.EqualError(t, err, "[planner:1227]Access denied; you need (at least one of) the RESTRICTED_USER_ADMIN privilege(s) for this operation") | ||
| tk4 := testkit.NewTestKit(t, store) | ||
| require.NoError(t, tk4.Session().Auth(&auth.UserIdentity{Username: "restricted_user", Hostname: "%"}, nil, nil, nil)) | ||
| err = tk4.ExecToErr("ALTER USER restricted_user_2 IDENTIFIED BY 'new_password'") | ||
| require.NoError(t, err) | ||
|
|
||
| // Accounts with RESTRICTED_USER_ADMIN are not allowed to be granted or revoked unless we also | ||
| // have the RESTRICTED_USER_ADMIN privilege | ||
| tk5 := testkit.NewTestKit(t, store) | ||
| require.NoError(t, tk5.Session().Auth(&auth.UserIdentity{Username: "normal_user", Hostname: "%"}, nil, nil, nil)) | ||
| err = tk5.ExecToErr("GRANT SELECT ON test.* TO restricted_user_3") | ||
| require.EqualError(t, err, "[planner:1227]Access denied; you need (at least one of) the RESTRICTED_USER_ADMIN privilege(s) for this operation") | ||
| err = tk5.ExecToErr("REVOKE SELECT ON test.* FROM restricted_user_3") | ||
| require.EqualError(t, err, "[planner:1227]Access denied; you need (at least one of) the RESTRICTED_USER_ADMIN privilege(s) for this operation") | ||
| tk6 := testkit.NewTestKit(t, store) | ||
| require.NoError(t, tk6.Session().Auth(&auth.UserIdentity{Username: "restricted_user", Hostname: "%"}, nil, nil, nil)) | ||
| err = tk6.ExecToErr("GRANT SELECT ON test.* TO restricted_user_3") | ||
| require.NoError(t, err) | ||
| err = tk6.ExecToErr("REVOKE SELECT ON test.* FROM restricted_user_3") | ||
| require.NoError(t, err) | ||
|
|
||
| // Accounts with RESTRICTED_USER_ADMIN are not allowed to be granted/revoked as a role unless we | ||
| // also have the RESTRICTED_USER_ADMIN privilege | ||
| tk7 := testkit.NewTestKit(t, store) | ||
| require.NoError(t, tk7.Session().Auth(&auth.UserIdentity{Username: "normal_user", Hostname: "%"}, nil, nil, nil)) | ||
| err = tk7.ExecToErr("GRANT restricted_user TO normal_user_1") | ||
| require.EqualError(t, err, "[planner:1227]Access denied; you need (at least one of) the RESTRICTED_USER_ADMIN privilege(s) for this operation") | ||
| err = tk7.ExecToErr("REVOKE restricted_user FROM normal_user_1") | ||
| require.EqualError(t, err, "[planner:1227]Access denied; you need (at least one of) the RESTRICTED_USER_ADMIN privilege(s) for this operation") | ||
| tk8 := testkit.NewTestKit(t, store) | ||
| require.NoError(t, tk8.Session().Auth(&auth.UserIdentity{Username: "restricted_user", Hostname: "%"}, nil, nil, nil)) | ||
| err = tk8.ExecToErr("GRANT restricted_user TO normal_user_1") | ||
| require.NoError(t, err) | ||
| err = tk8.ExecToErr("REVOKE restricted_user FROM normal_user_1") | ||
| require.NoError(t, err) | ||
| } | ||
|
|
||
| func TestProtectUserAndRoleWithRestrictedPrivileges(t *testing.T) { | ||
| testProtectUserAndRoleWithRestrictedPrivileges(t, sem.V1) | ||
| testProtectUserAndRoleWithRestrictedPrivileges(t, sem.V2) | ||
| } | ||
| >>>>>>> 3328284abcb (privileges,planner: implement checks for `RESTRICTED_USER_ADMIN` for granting privileges and roles (#64297)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Checking for merge conflict markers ==="
if rg '^(<<<<<<<|=======|>>>>>>>)' pkg/privilege/privileges/privileges_test.go 2>/dev/null; then
echo "Conflict markers found!"
else
echo "No conflict markers detected in file"
fi
echo ""
echo "=== Checking symbol usage (domain.GetDomain, vardef.AccelerateUserCreationUpdate) ==="
rg 'domain\.GetDomain|vardef\.AccelerateUserCreationUpdate' pkg/privilege/privileges/privileges_test.go -n || true
echo ""
echo "=== Checking import block ==="
head -50 pkg/privilege/privileges/privileges_test.go | grep -A 50 "^import" || trueRepository: pingcap/tidb
Length of output: 1514
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full import block ==="
awk '
BEGIN { in_import=0 }
/^import \(/ { in_import=1; print; next }
in_import && /^\)/ { print; in_import=0; next }
in_import { print }
' pkg/privilege/privileges/privileges_test.go
echo ""
echo "=== Searching for missing imports ==="
echo "Looking for 'domain' import:"
awk '
BEGIN { in_import=0 }
/^import \(/ { in_import=1; next }
in_import && /^\)/ { in_import=0; next }
in_import { print }
' pkg/privilege/privileges/privileges_test.go | grep -i "domain" || echo "NOT FOUND"
echo ""
echo "Looking for 'vardef' import:"
awk '
BEGIN { in_import=0 }
/^import \(/ { in_import=1; next }
in_import && /^\)/ { in_import=0; next }
in_import { print }
' pkg/privilege/privileges/privileges_test.go | grep -i "vardef" || echo "NOT FOUND"Repository: pingcap/tidb
Length of output: 1252
Resolve merge-conflict markers and add missing imports before merge.
Unresolved merge conflict markers remain at lines 2119-2120 and 2319 (<<<<<<<, =======, >>>>>>>), preventing compilation.
Additionally, the new code uses symbols that lack corresponding imports:
- Line 2159:
domain.GetDomain()— missing importgithub.com/pingcap/tidb/pkg/domain - Line 2172:
vardef.AccelerateUserCreationUpdate.Load()— missing importgithub.com/pingcap/tidb/pkg/sessionctx/vardef
Complete the merge resolution and add the required imports.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/privilege/privileges/privileges_test.go` around lines 2119 - 2319, The
file still contains unresolved merge markers (<<<<<<<, =======, >>>>>>>) around
the added tests (e.g., TestEnsureActiveUserCoverage and
TestSQLVariableAccelerateUserCreationUpdate); remove the conflict markers and
keep the intended merged blocks (all new test functions and the final
TestProtectUserAndRoleWithRestrictedPrivileges calls). Also add the missing
imports for domain.GetDomain and vardef.AccelerateUserCreationUpdate by
importing github.com/pingcap/tidb/pkg/domain and
github.com/pingcap/tidb/pkg/sessionctx/vardef in the test file's import block so
the symbols in TestSQLVariableAccelerateUserCreationUpdate compile.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an automated cherry-pick of #64297
What problem does this PR solve?
Issue Number: close #64295
Problem Summary:
The current implementation of SEM is not good enough (for both v1 and v2). We'll need to restrict the granting and revoking of roles which have
RESTRICTED_USER_ADMIN, or the user withROLE ADMINpermission can easy take the higher privilege.What changed and how does it work?
RESTRICTED_USER_ADMINare not allowed to be deletedRESTRICTED_USER_ADMINare not allowed to have their names modifiedRESTRICTED_USER_ADMINpermission are not allowed to change permissions.RESTRICTED_USER_ADMINattribute is not allowed to be used as a role.Users with
RESTRICTED_USER_ADMINare not limited by these four rules.Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
Bug Fixes
Tests