privileges,planner: implement checks for RESTRICTED_USER_ADMIN for granting privileges and roles (#64297)#66648
Open
ti-chi-bot wants to merge 1 commit intopingcap:release-7.5from
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 |
📝 WalkthroughWalkthroughAdds privilege-checking augmentation to the plan builder's grant and revoke statement handlers to enforce RESTRICTED_USER_ADMIN constraints, with comprehensive test coverage validating the enforcement across different scenarios and SEM versions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Planner
participant PrivilegeChecker
participant AdminChecker
Client->>Planner: Execute Grant/Revoke Role Statement
Planner->>PrivilegeChecker: Check basic privileges
PrivilegeChecker-->>Planner: Basic check passed
Planner->>AdminChecker: Verify RESTRICTED_USER_ADMIN constraint
alt User/Role has RESTRICTED_USER_ADMIN
AdminChecker-->>Planner: Access denied
Planner-->>Client: Error
else No restriction
AdminChecker-->>Planner: Constraint satisfied
Planner->>PrivilegeChecker: Apply privilege change
PrivilegeChecker-->>Client: Operation complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The changes introduce security-sensitive privilege logic modifications with merge-conflict markers requiring resolution, plus 200+ lines of multi-scenario test coverage including privilege checks across SEM versions and restricted-user constraints that demand careful validation. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
pkg/privilege/privileges/privileges_test.go (1)
2243-2312: AddRENAME USERassertions to complete restricted-user protection coverage.This helper validates drop/alter/grant/revoke and role-use restrictions, but it does not assert the rename restriction. Adding a deny/allow pair for
RENAME USERwould align test coverage with the intended behavior.✅ Suggested test additions
rootTk.MustExec("CREATE USER restricted_user_3") rootTk.MustExec("GRANT RESTRICTED_USER_ADMIN ON *.* TO restricted_user_3") + rootTk.MustExec("CREATE USER restricted_user_rename") + rootTk.MustExec("GRANT RESTRICTED_USER_ADMIN ON *.* TO restricted_user_rename") rootTk.MustExec("CREATE USER normal_user_1") defer sem.SwitchToSEMForTest(t, semVer)() @@ 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 renamed unless we also + // have the RESTRICTED_USER_ADMIN privilege. + err = tk3.ExecToErr("RENAME USER restricted_user_rename TO restricted_user_rename_tmp") + require.EqualError(t, err, "[planner:1227]Access denied; you need (at least one of) the RESTRICTED_USER_ADMIN privilege(s) for this operation") + err = tk4.ExecToErr("RENAME USER restricted_user_rename TO restricted_user_rename_tmp") + require.NoError(t, err)🤖 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 2243 - 2312, In testProtectUserAndRoleWithRestrictedPrivileges add assertions for RENAME USER similar to the DROP/ALTER blocks: create a new testkit (e.g., tk9) authenticated as normal_user and call ExecToErr("RENAME USER restricted_user_1 TO foo") and assert the same planner:1227 Access denied error with require.EqualError, then create another testkit (e.g., tk10) authenticated as restricted_user and call ExecToErr("RENAME USER restricted_user_1 TO foo") and assert success with require.NoError; place these deny/allow checks alongside the existing DROP USER and ALTER USER checks and use the same user names (restricted_user_1 or restricted_user_2) and ExecToErr/require helpers.
🤖 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 3622-3624: The call to appendVisitInfoIsRestrictedUser uses the
wrong argument order/extra parameter; change the invocation inside the for loop
to match the signature func appendVisitInfoIsRestrictedUser(visitInfo
[]visitInfo, sctx sessionctx.Context, user *auth.UserIdentity, priv string) by
removing the extra ctx arg and passing b.visitInfo, b.ctx, the user identity and
the privilege string, e.g. replace the current call with: b.visitInfo =
appendVisitInfoIsRestrictedUser(b.visitInfo, b.ctx, user,
"RESTRICTED_USER_ADMIN").
- Around line 3675-3677: The call to appendVisitInfoIsRestrictedUser in the loop
uses the wrong parameter order/types; open the appendVisitInfoIsRestrictedUser
function definition and update this call in the loop over raw.Users so the
arguments match the function signature exactly (pass the visitInfo value in the
correct position, the proper context variable, and the user identifier/type
strings in the expected order), and ensure its return (the updated visitInfo) is
assigned back to b.visitInfo; specifically adjust the call in the loop that
currently reads appendVisitInfoIsRestrictedUser(ctx, b.visitInfo, b.ctx,
user.User, "RESTRICTED_USER_ADMIN") to match the function's declared parameter
ordering and types.
- Around line 3645-3656: There is an unresolved Git merge conflict in the block
that constructs ErrSpecificAccessDenied and updates b.visitInfo; remove the
conflict markers (<<<<<<<, =======, >>>>>>>) and reconcile the two variants by
using the correct error symbol (either ErrSpecificAccessDenied or
plannererrors.ErrSpecificAccessDenied consistent with imports) and the correct
appendDynamicVisitInfo signature (string vs []string) used elsewhere in the
release-7.5 branch, then restore the additional restricted-user handling if
intended: call appendVisitInfoIsRestrictedUser for each entry in raw.Roles to
add RESTRICTED_USER_ADMIN checks and update b.visitInfo accordingly; ensure the
final code compiles and that b.visitInfo is updated only via the single resolved
appendDynamicVisitInfo call and the loop over raw.Roles.
- Around line 3664-3668: Call to appendVisitInfoIsRestrictedUser passes an extra
ctx argument that the function signature does not accept (same mismatch as in
GrantStmt); remove the leading ctx from the call inside the loop so the call
matches the function signature (i.e., call appendVisitInfoIsRestrictedUser with
b.visitInfo, b.ctx, &auth.UserIdentity{Username: role.Username, Hostname:
role.Hostname}, "RESTRICTED_USER_ADMIN").
In `@pkg/privilege/privileges/privileges_test.go`:
- Around line 2118-2318: The file has unresolved git conflict markers (e.g., the
"<<<<<<< HEAD", "=======", ">>>>>>> 3328284abcb" around
TestEnsureActiveUserCoverage/TestProtectUserAndRoleWithRestrictedPrivileges) and
is missing imports for domain.GetDomain and vardef.AccelerateUserCreationUpdate;
remove the conflict markers so only the intended test functions remain
(TestEnsureActiveUserCoverage, TestSQLVariableAccelerateUserCreationUpdate,
TestGrantOptionWithSEMv2, testProtectUserAndRoleWithRestrictedPrivileges,
TestProtectUserAndRoleWithRestrictedPrivileges) and add the missing imports
(github.com/pingcap/tidb/pkg/domain and
github.com/pingcap/tidb/pkg/sessionctx/vardef) to the file's import block so
references to domain.GetDomain and vardef.AccelerateUserCreationUpdate compile.
---
Nitpick comments:
In `@pkg/privilege/privileges/privileges_test.go`:
- Around line 2243-2312: In testProtectUserAndRoleWithRestrictedPrivileges add
assertions for RENAME USER similar to the DROP/ALTER blocks: create a new
testkit (e.g., tk9) authenticated as normal_user and call ExecToErr("RENAME USER
restricted_user_1 TO foo") and assert the same planner:1227 Access denied error
with require.EqualError, then create another testkit (e.g., tk10) authenticated
as restricted_user and call ExecToErr("RENAME USER restricted_user_1 TO foo")
and assert success with require.NoError; place these deny/allow checks alongside
the existing DROP USER and ALTER USER checks and use the same user names
(restricted_user_1 or restricted_user_2) and ExecToErr/require helpers.
Comment on lines
+3622
to
+3624
| 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.
Function signature mismatch will cause compilation error.
The call to appendVisitInfoIsRestrictedUser passes 5 arguments with ctx as the first parameter, but the function definition at Line 3795 only accepts 4 parameters:
func appendVisitInfoIsRestrictedUser(visitInfo []visitInfo, sctx sessionctx.Context, user *auth.UserIdentity, priv string) []visitInfoCompare with existing usage at Line 3662 (which is correct):
b.visitInfo = appendVisitInfoIsRestrictedUser(b.visitInfo, b.ctx, user, "RESTRICTED_USER_ADMIN")🐛 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")
}🤖 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 3622 - 3624, The call to
appendVisitInfoIsRestrictedUser uses the wrong argument order/extra parameter;
change the invocation inside the for loop to match the signature func
appendVisitInfoIsRestrictedUser(visitInfo []visitInfo, sctx sessionctx.Context,
user *auth.UserIdentity, priv string) by removing the extra ctx arg and passing
b.visitInfo, b.ctx, the user identity and the privilege string, e.g. replace the
current call with: b.visitInfo = appendVisitInfoIsRestrictedUser(b.visitInfo,
b.ctx, user, "RESTRICTED_USER_ADMIN").
Comment on lines
+3645
to
+3656
| <<<<<<< HEAD | ||
| err := ErrSpecificAccessDenied.GenWithStackByArgs("SUPER or ROLE_ADMIN") | ||
| b.visitInfo = appendDynamicVisitInfo(b.visitInfo, "ROLE_ADMIN", false, err) | ||
| ======= | ||
| err := plannererrors.ErrSpecificAccessDenied.GenWithStackByArgs("SUPER or ROLE_ADMIN") | ||
| 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.
Unresolved merge conflict will cause build failure.
The code contains Git merge conflict markers (<<<<<<< HEAD, =======, >>>>>>>) that must be resolved before this PR can be merged. The conflict involves:
- Different error type imports:
ErrSpecificAccessDeniedvsplannererrors.ErrSpecificAccessDenied - Different
appendDynamicVisitInfosignatures:"ROLE_ADMIN"(string) vs[]string{"ROLE_ADMIN"}(slice)
Please resolve the conflict by choosing the appropriate implementation for the release-7.5 branch and removing the conflict markers.
🤖 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 3645 - 3656, There is an
unresolved Git merge conflict in the block that constructs
ErrSpecificAccessDenied and updates b.visitInfo; remove the conflict markers
(<<<<<<<, =======, >>>>>>>) and reconcile the two variants by using the correct
error symbol (either ErrSpecificAccessDenied or
plannererrors.ErrSpecificAccessDenied consistent with imports) and the correct
appendDynamicVisitInfo signature (string vs []string) used elsewhere in the
release-7.5 branch, then restore the additional restricted-user handling if
intended: call appendVisitInfoIsRestrictedUser for each entry in raw.Roles to
add RESTRICTED_USER_ADMIN checks and update b.visitInfo accordingly; ensure the
final code compiles and that b.visitInfo is updated only via the single resolved
appendDynamicVisitInfo call and the loop over raw.Roles.
Comment on lines
+3664
to
+3668
| // 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") | ||
| } |
There was a problem hiding this comment.
Function signature mismatch - same issue as GrantStmt.
The call passes ctx as the first argument, but appendVisitInfoIsRestrictedUser doesn't accept a context.Context parameter.
🐛 Proposed fix
// 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},
+ 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
| // 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") | |
| } | |
| // If any of the roles are RESTRICTED, require RESTRICTED_ROLE_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 3664 - 3668, Call to
appendVisitInfoIsRestrictedUser passes an extra ctx argument that the function
signature does not accept (same mismatch as in GrantStmt); remove the leading
ctx from the call inside the loop so the call matches the function signature
(i.e., call appendVisitInfoIsRestrictedUser with b.visitInfo, b.ctx,
&auth.UserIdentity{Username: role.Username, Hostname: role.Hostname},
"RESTRICTED_USER_ADMIN").
Comment on lines
+3675
to
+3677
| 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.
Function signature mismatch - same issue as above.
🐛 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")
}📝 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 3675 - 3677, The call to
appendVisitInfoIsRestrictedUser in the loop uses the wrong parameter
order/types; open the appendVisitInfoIsRestrictedUser function definition and
update this call in the loop over raw.Users so the arguments match the function
signature exactly (pass the visitInfo value in the correct position, the proper
context variable, and the user identifier/type strings in the expected order),
and ensure its return (the updated visitInfo) is assigned back to b.visitInfo;
specifically adjust the call in the loop that currently reads
appendVisitInfoIsRestrictedUser(ctx, b.visitInfo, b.ctx, user.User,
"RESTRICTED_USER_ADMIN") to match the function's declared parameter ordering and
types.
Comment on lines
+2118
to
+2318
| <<<<<<< 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
file="pkg/privilege/privileges/privileges_test.go"
echo "=== 1) Checking for unresolved merge markers ==="
rg -n '^(<<<<<<<|=======|>>>>>>>)' "$file" || echo "No merge markers found"
echo ""
echo "=== 2) Checking for newly used identifiers ==="
rg -n '\bdomain\.GetDomain\b|\bvardef\.AccelerateUserCreationUpdate\b' "$file" || echo "No identifier usage found"
echo ""
echo "=== 3) Checking for required imports ==="
rg -n '"github.com/pingcap/tidb/pkg/domain"|"github.com/pingcap/tidb/pkg/sessionctx/vardef"' "$file" || echo "No required imports found"
echo ""
echo "=== 4) Checking import block structure (first 50 lines) ==="
head -50 "$file" | cat -nRepository: pingcap/tidb
Length of output: 2518
Remove merge-conflict markers and add missing imports; this file will not compile as-is.
The code block contains unresolved Git conflict markers at lines 2118, 2119, and 2318. Additionally, line 2158 uses domain.GetDomain() and line 2171 uses vardef.AccelerateUserCreationUpdate, but neither github.com/pingcap/tidb/pkg/domain nor github.com/pingcap/tidb/pkg/sessionctx/vardef are imported.
Fix merge conflict and add imports
Remove the conflict markers (lines 2118–2119 and 2318) and add the missing imports to the import block.
🤖 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 2118 - 2318, The
file has unresolved git conflict markers (e.g., the "<<<<<<< HEAD", "=======",
">>>>>>> 3328284abcb" around
TestEnsureActiveUserCoverage/TestProtectUserAndRoleWithRestrictedPrivileges) and
is missing imports for domain.GetDomain and vardef.AccelerateUserCreationUpdate;
remove the conflict markers so only the intended test functions remain
(TestEnsureActiveUserCoverage, TestSQLVariableAccelerateUserCreationUpdate,
TestGrantOptionWithSEMv2, testProtectUserAndRoleWithRestrictedPrivileges,
TestProtectUserAndRoleWithRestrictedPrivileges) and add the missing imports
(github.com/pingcap/tidb/pkg/domain and
github.com/pingcap/tidb/pkg/sessionctx/vardef) to the file's import block so
references to domain.GetDomain and vardef.AccelerateUserCreationUpdate compile.
|
@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. |
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