*: parser support for MySQL dual passwords (RETAIN CURRENT PASSWORD / DISCARD OLD PASSWORD)#68028
Open
takaidohigasi wants to merge 18 commits into
Open
*: parser support for MySQL dual passwords (RETAIN CURRENT PASSWORD / DISCARD OLD PASSWORD)#68028takaidohigasi wants to merge 18 commits into
takaidohigasi wants to merge 18 commits into
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-tests-checked
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds MySQL-compatible dual-password support: parser recognizes ChangesDual-password feature
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Parser as Parser
participant Executor as Executor\n(pkg/executor/simple.go)
participant UserTable as mysql.user
participant Cache as Privilege Cache\n(pkg/privilege/privileges/cache.go)
Client->>Parser: ALTER USER 'u' IDENTIFIED BY 'new' RETAIN CURRENT PASSWORD
Parser->>Executor: AST (DualPasswordOption / RetainCurrentPassword)
Executor->>UserTable: SELECT authentication_string FOR UPDATE
UserTable-->>Executor: current_auth_string
Executor->>UserTable: UPDATE authentication_string=new_hash, user_attributes=JSON_MERGE/REMOVE($.additional_password)
UserTable-->>Executor: OK
Executor-->>Client: OK
sequenceDiagram
participant Client as Client
participant Auth as Authenticator\n(pkg/privilege/privileges/privileges.go)
participant Cache as Privilege Cache
participant Primary as Primary Hash Check
participant Secondary as Secondary Hash Check
Client->>Auth: CONNECT user / password
Auth->>Cache: Load UserRecord (AuthenticationString, AdditionalAuthenticationString)
Cache-->>Auth: UserRecord
Auth->>Primary: check(password, AuthenticationString)
Primary-->>Auth: FAIL (retryable)
Auth->>Secondary: check(password, AdditionalAuthenticationString)
Secondary-->>Auth: SUCCESS
Auth-->>Client: Authenticated (used retained password)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 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)
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
Bot
added
contribution
|
Hi @takaidohigasi. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
Hi @takaidohigasi. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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. |
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
ti-chi-bot
Bot
added
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
pkg/executor/test/passwordtest/BUILD.bazel (1)
7-12: Confirm Bazel metadata was regenerated.Since a new Go test source file is added to
srcs, please ensuremake bazel_preparewas run and any resulting metadata changes are included in this PR. Theshard_countbump from 9 to 17 is a large jump for adding a single test file — if it was chosen empirically to keep wall-time reasonable that's fine, otherwise consider starting with a smaller bump (e.g. 12) to avoid unnecessary parallelism overhead.Based on learnings: "MUST run
make bazel_prepareand include resulting Bazel metadata changes in the PR when adding/moving/renaming/removing Go files, changing Bazel files, updating Bazel test targets".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/test/passwordtest/BUILD.bazel` around lines 7 - 12, Regenerate and commit Bazel metadata by running make bazel_prepare because a new Go test file ("dual_password_test.go") was added to the srcs array in BUILD.bazel; include the resulting changes in this PR and ensure the BUILD.bazel entry reflects those updates, and if the shard_count change (currently 17) was not chosen empirically, revert it to a smaller increment (e.g., 12) or add a brief justification comment in the PR explaining why 17 is required to keep wall-time reasonable.pkg/privilege/privileges/cache.go (1)
1065-1075: LGTM — follows the existing extraction pattern.Extraction mirrors the sibling
$.resource_groupblock (lines 1054-1064): parse path expr,Extract,Unquote,strings.Clone. This keeps the decoder style consistent.Optional nit (not blocking): the three
ParseJSONPathExprcalls ($.metadata.email,$.resource_group,$.additional_password) are re-parsed per row on every cache reload. If the hot path ever becomes a concern, these could be hoisted to package-levelvarinitializers. Out of scope for this PR — existing code already pays this cost.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/privilege/privileges/cache.go` around lines 1065 - 1075, No change required for correctness—the added extraction of `$.additional_password` (in the block using types.ParseJSONPathExpr, bj.Extract, additionalBJ.Unquote and strings.Clone feeding value.AdditionalAuthenticationString) correctly mirrors the existing `$.resource_group` pattern; if you want to address the optional nit about repeated parsing on each cache reload, hoist the repeated calls to types.ParseJSONPathExpr for `$.metadata.email`, `$.resource_group`, and `$.additional_password` into package-level var initializers (e.g., pre-parse into vars like pkgEmailPathExpr, pkgResourceGroupPathExpr, pkgAdditionalPasswordPathExpr) and replace the per-row ParseJSONPathExpr calls in the decoder functions with those pre-parsed vars to avoid re-parsing on the hot path.pkg/parser/parser_test.go (1)
5260-5266: Add negative parser cases for unsupportedCREATE USER ... RETAIN/DISCARDcombinations.This block covers accepted syntax well, but it doesn’t pin the rejection behavior for
CREATE USERwith dual-password clauses. Adding explicitok=falserows here would guard the compatibility contract against parser regressions.Proposed test additions
+ {"CREATE USER 'u1'@'%' IDENTIFIED BY 'new' RETAIN CURRENT PASSWORD", false, ""}, + {"CREATE USER 'u1'@'%' DISCARD OLD PASSWORD", false, ""},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/parser_test.go` around lines 5260 - 5266, Add negative parser test rows next to the dual-password block in parser_test.go: add CREATE USER variants using RETAIN CURRENT PASSWORD and DISCARD OLD PASSWORD (e.g. "CREATE USER 'u1'@'%' IDENTIFIED BY 'x' RETAIN CURRENT PASSWORD", "CREATE USER 'u1'@'%' IDENTIFIED WITH 'mysql_native_password' BY 'x' DISCARD OLD PASSWORD", etc.) with ok=false so the parser is expected to reject these combinations; place them alongside the existing ALTER/SET examples and reference the clause text (CREATE USER, RETAIN CURRENT PASSWORD, DISCARD OLD PASSWORD) so future diffs can find and maintain these negative cases.pkg/privilege/privileges/privileges.go (1)
700-711: Consider emitting a log when the secondary password succeeds.During a credential rotation, operators typically want to know which accounts have already switched to the primary vs. are still using the retained secondary, so they can safely run
ALTER USER ... DISCARD OLD PASSWORDat the end. As implemented, a successful dual-password fallback is indistinguishable from a regular login in the logs. A single info-level log (or metric counter) on the fallback-success path would materially aid rotation workflows without affecting correctness.💡 Suggested hook
ok, retryable := checkHash(pwd) - if !ok && retryable && len(record.AdditionalAuthenticationString) > 0 { - // MySQL-compatible dual-password fallback: try the secondary password - // stored in user_attributes.$.additional_password with the same plugin. - ok, _ = checkHash(record.AdditionalAuthenticationString) - } + if !ok && retryable && len(record.AdditionalAuthenticationString) > 0 { + // MySQL-compatible dual-password fallback: try the secondary password + // stored in user_attributes.$.additional_password with the same plugin. + ok, _ = checkHash(record.AdditionalAuthenticationString) + if ok { + logutil.BgLogger().Info("authenticated using retained (secondary) password", + zap.String("authUser", authUser), zap.String("authHost", authHost), + zap.String("plugin", record.AuthPlugin)) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/privilege/privileges/privileges.go` around lines 700 - 711, When the dual-password fallback path in the switch for record.AuthPlugin (where checkHash(pwd) fails and checkHash(record.AdditionalAuthenticationString) returns ok) succeeds, emit an info-level log or increment a metric to indicate the fallback was used; specifically, after the successful checkHash(record.AdditionalAuthenticationString) result is true, record an informational message/metric including user.Username, user.Hostname and record.AuthPlugin (or otherwise tag by username/host/plugin) so operators can distinguish fallback logins from primary-password logins. Ensure the log/metric is lightweight and only executed on the fallback-success branch (i.e., immediately after ok, _ = checkHash(record.AdditionalAuthenticationString) and before returning success).pkg/executor/simple.go (3)
923-932: LGTM on the mutual-exclusion check.Correctly rejects
RETAIN CURRENT PASSWORDandDISCARD OLD PASSWORDspecified together. One nit: MySQL surfaces this asER_DUPLICATE_PASSWORD_SPECIFIED_KEYWORDS(3864). Using a specific error code would improve client compatibility, but this is a minor refinement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/simple.go` around lines 923 - 932, The mutual-exclusion check currently returns a generic error; replace the generic errors.Errorf call that checks info.retainCurrentPassword && info.discardOldPassword with the MySQL-specific error for duplicate password keywords (ER_DUPLICATE_PASSWORD_SPECIFIED_KEYWORDS / code 3864) so clients see the canonical error. Locate the clause that returns errors.Errorf("RETAIN CURRENT PASSWORD and DISCARD OLD PASSWORD can not be used together") and change it to construct and return the MySQL error (e.g., mysql.NewErrf(mysql.ER_DUPLICATE_PASSWORD_SPECIFIED_KEYWORDS, "RETAIN CURRENT PASSWORD and DISCARD OLD PASSWORD cannot be used together") or the equivalent project-specific errno helper) so the specific error code 3864 is emitted.
2015-2031: Recommended refactor: extract theadditional_passwordJSON builder.The logic to read the current
authentication_stringand JSON-encode it into anadditional_passwordattribute is duplicated betweenexecuteAlterUser(lines 2018–2031) andexecuteSetPwd(lines 2722–2737). Extracting a small helper keeps the two code paths aligned if either the encoding rule or the SQL-injection assumption changes.♻️ Proposed helper
// buildAdditionalPasswordJSON reads the user's current authentication_string and // returns a JSON object fragment `{"additional_password": "<hash>"}` suitable // for feeding into json_merge_patch on user_attributes. func buildAdditionalPasswordJSON(ctx context.Context, sqlExecutor sqlexec.SQLExecutor, user, host string) (string, error) { oldPwd, err := readAuthenticationString(ctx, sqlExecutor, user, host) if err != nil { return "", err } encoded, err := json.Marshal(oldPwd) if err != nil { return "", err } return fmt.Sprintf(`{"additional_password": %s}`, encoded), nil }Then use it from both call sites.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/simple.go` around lines 2015 - 2031, Extract the duplicated logic that reads mysql.user.authentication_string and JSON-encodes it into a helper named buildAdditionalPasswordJSON(ctx, sqlExecutor, user, host) that returns the JSON fragment `{"additional_password": "<hash>"}` (string, error); replace the inline code in executeAlterUser and executeSetPwd with calls to this helper, returning errors unchanged on failure, so both call sites use the same encoding and error handling via readAuthenticationString and json.Marshal.
2533-2564: Minor:readAuthenticationStringsilently returns""on missing row.If the target
user@hostis not present, the function returns("", nil)rather than an error. Every production caller already verifies existence viauserExistsInternalbeforehand, so this is defensive — but it also means that any future caller that forgets the existence check will silently store an empty secondary password. Consider returning a sentinel error (or a distinctfound bool) to make the precondition explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/simple.go` around lines 2533 - 2564, readAuthenticationString currently returns ("", nil) when the user row is missing which can silently propagate an empty secondary password; change the function signature to readAuthenticationString(ctx context.Context, sqlExecutor sqlexec.SQLExecutor, name, host string) (string, bool, error) and return (authString, true, nil) when found and ("", false, nil) when not found; update all call sites (places that currently call readAuthenticationString, e.g., the RETAIN CURRENT PASSWORD handling logic and any callers that assume existence) to check the boolean before using the returned string and handle the not-found case (or convert it into an explicit error there) so a missing user no longer produces a silent empty password.pkg/executor/test/passwordtest/dual_password_test.go (1)
27-39: Recommended: avoid creating a secondMockStoreper authentication attempt.
rootTKcallstestkit.CreateMockStore(t)per test (fine), butauthAsthen creates a freshtestkit.TestKiton the same store viatk.Session().GetStore()each call. That's OK. However,rootTKreturns acleanup func() {}that is a no-op — callersdefer cleanup()it, which is dead code. Either wire the cleanup to something meaningful (e.g.,tk.Session().Close()or aDROP USER), or drop the return value and itsdefer cleanup()call-sites to reduce boilerplate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/test/passwordtest/dual_password_test.go` around lines 27 - 39, The rootTK helper currently returns a no-op cleanup which callers defer, creating dead code; update rootTK or its call-sites: either (A) make cleanup meaningful by closing the TestKit/session (call tk.Session().Close() or equivalent) and return that cleanup from rootTK so deferred cleanup actually releases resources, or (B) remove the cleanup return (change signature of rootTK to return only *testkit.TestKit) and remove the deferred cleanup calls where rootTK is used; ensure authAs continues to create sub TestKit via tk.Session().GetStore() and keep symbols rootTK, authAs, TestKit and session handling consistent.
🤖 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/executor/simple.go`:
- Around line 1817-1843: When handling RETAIN CURRENT PASSWORD in
executeAlterUser and executeSetPwd, ensure the primary password read via
readAuthenticationString is not empty before encoding/writing it as
additional_password; specifically, after capturing oldPwd in executeAlterUser
(near where readAuthenticationString is called) and after the
readAuthenticationString call in executeSetPwd, check if the returned string is
empty and, if so, return an error rejecting RETAIN CURRENT PASSWORD (e.g., an
errors.Errorf with a message like "Current password can not be retained for user
'%s'@'%s' because primary password is empty", using the same username/hostname
symbols used elsewhere); do not proceed to set additional_password when the
primary is empty.
In `@pkg/executor/test/passwordtest/dual_password_test.go`:
- Around line 156-163: Add an explicit negative assertion that DISCARD OLD
PASSWORD fails without APPLICATION_PASSWORD_ADMIN before granting the privilege:
in the test dual_password_test.go, call adminTK2.MustExec or use authAs to
attempt "ALTER USER dpvictim DISCARD OLD PASSWORD" (or attempt authAs after a
DISCARD attempt) and assert an error prior to granting the privilege, mirroring
the existing negative RETAIN case around lines 145–147; this ensures the
discardOldPassword guard in simple.go is actually enforced and regression-safe
before you grant the permission and exercise the success path with
adminTK2.MustExec("ALTER USER dpvictim DISCARD OLD PASSWORD") and subsequent
authAs checks.
- Around line 166-186: The test TestDualPasswordCachingSha2Password currently
claims to verify "decoded by the privilege cache" but only asserts persistence;
either update the test comment/docstring to state this is a persistence-only
check (no authentication attempted), or add an actual authentication check
implemented without using authAs() / sha1Password() (which only works for
mysql_native_password) — e.g., perform a real client login flow that uses
caching_sha2_password or call the server-side CheckHashingPassword test helpers
(see CheckHashingPassword in pkg/parser/auth/caching_sha2_test.go) to validate
the stored hashes; reference TestDualPasswordRetainAndDiscard and
TestDualPasswordSetPasswordRetain for expected auth patterns when choosing to
add authentication verification.
In `@pkg/privilege/privileges/privileges.go`:
- Around line 688-693: The error log is misleading because the block handling
mysql.AuthCachingSha2Password and mysql.AuthTiDBSM3Password always logs "Failed
to check caching_sha2_password"; update the log to include the actual plugin and
user context and avoid hardcoding the plugin name: when
auth.CheckHashingPassword returns an error, call logutil.BgLogger().Error with a
neutral message (e.g., "Failed to check hashed password") and add fields
zap.String("auth_plugin", record.AuthPlugin) and zap.String("auth_user",
record.User or authUser variable) plus zap.Error(err) so operators can see which
plugin (and user) failed; keep returning ok, true as before.
---
Nitpick comments:
In `@pkg/executor/simple.go`:
- Around line 923-932: The mutual-exclusion check currently returns a generic
error; replace the generic errors.Errorf call that checks
info.retainCurrentPassword && info.discardOldPassword with the MySQL-specific
error for duplicate password keywords (ER_DUPLICATE_PASSWORD_SPECIFIED_KEYWORDS
/ code 3864) so clients see the canonical error. Locate the clause that returns
errors.Errorf("RETAIN CURRENT PASSWORD and DISCARD OLD PASSWORD can not be used
together") and change it to construct and return the MySQL error (e.g.,
mysql.NewErrf(mysql.ER_DUPLICATE_PASSWORD_SPECIFIED_KEYWORDS, "RETAIN CURRENT
PASSWORD and DISCARD OLD PASSWORD cannot be used together") or the equivalent
project-specific errno helper) so the specific error code 3864 is emitted.
- Around line 2015-2031: Extract the duplicated logic that reads
mysql.user.authentication_string and JSON-encodes it into a helper named
buildAdditionalPasswordJSON(ctx, sqlExecutor, user, host) that returns the JSON
fragment `{"additional_password": "<hash>"}` (string, error); replace the inline
code in executeAlterUser and executeSetPwd with calls to this helper, returning
errors unchanged on failure, so both call sites use the same encoding and error
handling via readAuthenticationString and json.Marshal.
- Around line 2533-2564: readAuthenticationString currently returns ("", nil)
when the user row is missing which can silently propagate an empty secondary
password; change the function signature to readAuthenticationString(ctx
context.Context, sqlExecutor sqlexec.SQLExecutor, name, host string) (string,
bool, error) and return (authString, true, nil) when found and ("", false, nil)
when not found; update all call sites (places that currently call
readAuthenticationString, e.g., the RETAIN CURRENT PASSWORD handling logic and
any callers that assume existence) to check the boolean before using the
returned string and handle the not-found case (or convert it into an explicit
error there) so a missing user no longer produces a silent empty password.
In `@pkg/executor/test/passwordtest/BUILD.bazel`:
- Around line 7-12: Regenerate and commit Bazel metadata by running make
bazel_prepare because a new Go test file ("dual_password_test.go") was added to
the srcs array in BUILD.bazel; include the resulting changes in this PR and
ensure the BUILD.bazel entry reflects those updates, and if the shard_count
change (currently 17) was not chosen empirically, revert it to a smaller
increment (e.g., 12) or add a brief justification comment in the PR explaining
why 17 is required to keep wall-time reasonable.
In `@pkg/executor/test/passwordtest/dual_password_test.go`:
- Around line 27-39: The rootTK helper currently returns a no-op cleanup which
callers defer, creating dead code; update rootTK or its call-sites: either (A)
make cleanup meaningful by closing the TestKit/session (call
tk.Session().Close() or equivalent) and return that cleanup from rootTK so
deferred cleanup actually releases resources, or (B) remove the cleanup return
(change signature of rootTK to return only *testkit.TestKit) and remove the
deferred cleanup calls where rootTK is used; ensure authAs continues to create
sub TestKit via tk.Session().GetStore() and keep symbols rootTK, authAs, TestKit
and session handling consistent.
In `@pkg/parser/parser_test.go`:
- Around line 5260-5266: Add negative parser test rows next to the dual-password
block in parser_test.go: add CREATE USER variants using RETAIN CURRENT PASSWORD
and DISCARD OLD PASSWORD (e.g. "CREATE USER 'u1'@'%' IDENTIFIED BY 'x' RETAIN
CURRENT PASSWORD", "CREATE USER 'u1'@'%' IDENTIFIED WITH 'mysql_native_password'
BY 'x' DISCARD OLD PASSWORD", etc.) with ok=false so the parser is expected to
reject these combinations; place them alongside the existing ALTER/SET examples
and reference the clause text (CREATE USER, RETAIN CURRENT PASSWORD, DISCARD OLD
PASSWORD) so future diffs can find and maintain these negative cases.
In `@pkg/privilege/privileges/cache.go`:
- Around line 1065-1075: No change required for correctness—the added extraction
of `$.additional_password` (in the block using types.ParseJSONPathExpr,
bj.Extract, additionalBJ.Unquote and strings.Clone feeding
value.AdditionalAuthenticationString) correctly mirrors the existing
`$.resource_group` pattern; if you want to address the optional nit about
repeated parsing on each cache reload, hoist the repeated calls to
types.ParseJSONPathExpr for `$.metadata.email`, `$.resource_group`, and
`$.additional_password` into package-level var initializers (e.g., pre-parse
into vars like pkgEmailPathExpr, pkgResourceGroupPathExpr,
pkgAdditionalPasswordPathExpr) and replace the per-row ParseJSONPathExpr calls
in the decoder functions with those pre-parsed vars to avoid re-parsing on the
hot path.
In `@pkg/privilege/privileges/privileges.go`:
- Around line 700-711: When the dual-password fallback path in the switch for
record.AuthPlugin (where checkHash(pwd) fails and
checkHash(record.AdditionalAuthenticationString) returns ok) succeeds, emit an
info-level log or increment a metric to indicate the fallback was used;
specifically, after the successful
checkHash(record.AdditionalAuthenticationString) result is true, record an
informational message/metric including user.Username, user.Hostname and
record.AuthPlugin (or otherwise tag by username/host/plugin) so operators can
distinguish fallback logins from primary-password logins. Ensure the log/metric
is lightweight and only executed on the fallback-success branch (i.e.,
immediately after ok, _ = checkHash(record.AdditionalAuthenticationString) and
before returning success).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c6b4fa48-d776-478f-a021-d583bc8f49f8
📒 Files selected for processing (12)
pkg/executor/simple.gopkg/executor/test/passwordtest/BUILD.bazelpkg/executor/test/passwordtest/dual_password_test.gopkg/executor/windows/BUILD.bazelpkg/parser/ast/misc.gopkg/parser/keywords.gopkg/parser/misc.gopkg/parser/parser.gopkg/parser/parser.ypkg/parser/parser_test.gopkg/privilege/privileges/cache.gopkg/privilege/privileges/privileges.go
💤 Files with no reviewable changes (1)
- pkg/executor/windows/BUILD.bazel
Contributor
Author
|
I will add minor fixes and add e2e tests |
takaidohigasi
force-pushed
the
feature/dual-password
branch
2 times, most recently
from
07be4ee to
7386798
Compare
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
Contributor
Author
|
Thanks @coderabbitai for the thorough review. Addressed in Actionable (4/4 fixed)
Nitpicks (5/8 applied, 3 justified)Applied:
Justified / skipped:
Verification
@coderabbitai review |
|
The three justified skips are all reasonable:
Triggering a re-review of the force-pushed changes now. 🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tests/integrationtest/t/executor/dual_password.test (2)
41-60: Please cover the self-service privilege path as well.The PR contract says
APPLICATION_PASSWORD_ADMINis only required when changing other users. This test only exercises the cross-user branch, so a regression that accidentally gates self-serviceRETAIN/DISCARDwould slip through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integrationtest/t/executor/dual_password.test` around lines 41 - 60, Add a self-service test that ensures APPLICATION_PASSWORD_ADMIN is not required when a user alters their own password with RETAIN/DISCARD: use the existing dpvictim user and connect as dpvictim (e.g., connect (cvictim, 127.0.0.1, dpvictim, v1, test)), then run alter user dpvictim identified by 'v2' retain current password (and a DISCARD variant) and assert success (and check mysql.user.user_attributes like the cross-user test does); keep the cross-user tests for dpadmin unchanged so both paths (self-service via dpvictim and cross-user via dpadmin) are covered to catch regressions that might gate self-service operations.
7-39: Add an end-to-end login check for the retained-password window.These assertions prove JSON storage, but they do not validate the user-visible behavior added in
pkg/privilege/privileges/privileges.go:594-749: authenticating with the old password beforeDISCARD OLD PASSWORD, and rejecting it afterward. A regression inConnectionVerificationwould still pass this test.pkg/executor/simple.go (3)
2013-2024: Avoid string-surgery on JSON output ofbuildAdditionalPasswordJSON.Line 2023 peels the outer
{/}off the helper's return value so the inner pair can be concatenated with the othernewAttributesentries. This tight coupling between the helper's JSON-object shape and the caller's string-joining is fragile: any future change to the helper (e.g., adding a sibling key, pretty-printing, or different encoding) silently breaks the concatenation.Prefer having the helper return just the inner fragment (or the raw encoded hash) and let the caller compose the object. For example:
♻️ Alternative API
-func buildAdditionalPasswordJSON(ctx context.Context, sqlExecutor sqlexec.SQLExecutor, name, host string) (string, error) { +// buildAdditionalPasswordEntry returns `"additional_password": "<hash>"` suitable +// for inclusion in a user_attributes JSON object. +func buildAdditionalPasswordEntry(ctx context.Context, sqlExecutor sqlexec.SQLExecutor, name, host string) (string, error) { oldPwd, found, err := readAuthenticationString(ctx, sqlExecutor, name, host) ... - return fmt.Sprintf(`{"additional_password": %s}`, encoded), nil + return fmt.Sprintf(`"additional_password": %s`, encoded), nil }Then the caller becomes:
- attrObj, err := buildAdditionalPasswordJSON(ctx, sqlExecutor, spec.User.Username, spec.User.Hostname) + entry, err := buildAdditionalPasswordEntry(ctx, sqlExecutor, spec.User.Username, spec.User.Hostname) if err != nil { return err } - newAttributes = append(newAttributes, strings.TrimSuffix(strings.TrimPrefix(attrObj, "{"), "}")) + newAttributes = append(newAttributes, entry)Same applies to the
executeSetPwdcaller on line 2751 — it keeps the full-object form because it passes the JSON directly tojson_merge_patch, so either keep both forms (rename helpers accordingly) or construct the{ ... }wrapper at the single call site that needs it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/simple.go` around lines 2013 - 2024, The code performs fragile string-surgery on the JSON returned by buildAdditionalPasswordJSON (used when plOptions.retainCurrentPassword is true) by trimming outer braces and appending to newAttributes; change the API so buildAdditionalPasswordJSON returns either the inner fragment or the raw password/hash (or a structured map) and update callers accordingly: have buildAdditionalPasswordJSON return a well-defined fragment (or value) and then compose the surrounding JSON object at the call site that needs it (the block that appends to newAttributes and the executeSetPwd caller), removing any TrimPrefix/TrimSuffix operations and instead building the JSON explicitly from the returned fragment/value to avoid brittle string concatenation.
1103-1110: Use dedicated, code-carrying errors for the CREATE USER rejection.
errors.Errorfproduces a generic error without a MySQL error code, so clients see a plain string rather than the numeric code/SQLSTATE they'd get from MySQL. The rest of this PR already introduces named errors inexeerrorsfor the dual-password contract (empty-new, empty-primary, plugin-change). Consider adding analogous ones (or reusing an existing "clause not supported in this statement" error) here and at lines 1826 and 2682 for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/simple.go` around lines 1103 - 1110, Replace the generic errors.Errorf calls that reject plOptions.retainCurrentPassword and plOptions.discardOldPassword with dedicated executable errors from the exeerrors package (e.g., a new exeerrors.ErrUnsupportedClauseForCreateUser or reuse an existing "clause not supported in this statement" error) so the client receives proper MySQL error codes/SQLSTATE; update the two other analogous rejection sites (the other checks handling retainCurrentPassword/discardOldPassword elsewhere in this file) to use the same exeerrors symbol for consistency and ensure callers return that error value.
2013-2041: Twouser_attributes=assignments in one UPDATE — consider combining.When
DISCARD OLD PASSWORD(or an auth-plugin change) coincides with other attribute changes (metadata, resource_group, password_locking), the generated UPDATE ends up with two assignments touser_attributes: onejson_merge_patch(line 2030) and onejson_remove(line 2040). This only produces the intended "merge then remove" result because MySQL evaluates same-row assignments left-to-right so the second expression sees the value written by the first — a subtle contract to rely on, and fragile to future reordering/refactors.Prefer composing the two ops into a single assignment, e.g.:
♻️ Suggested consolidation
- if length := len(newAttributes); length > 0 { - if length > 1 || passwordLockingStr == "" { - passwordLockingInfo.containsNoOthers = false - } - newAttributesStr := fmt.Sprintf("{%s}", strings.Join(newAttributes, ",")) - fields = append(fields, alterField{"user_attributes=json_merge_patch(coalesce(user_attributes, '{}'), %?)", newAttributesStr}) - } - // DISCARD OLD PASSWORD removes the secondary password. - // MySQL also silently drops the secondary when the auth plugin is changed; - // detect that here and do the same. - dropSecondary := plOptions.discardOldPassword - if !dropSecondary && spec.AuthOpt != nil && spec.AuthOpt.AuthPlugin != "" && spec.AuthOpt.AuthPlugin != currentAuthPlugin { - dropSecondary = true - } - if dropSecondary && !plOptions.retainCurrentPassword { - fields = append(fields, alterField{"user_attributes=json_remove(coalesce(user_attributes, '{}'), '$.additional_password')", nil}) - } + dropSecondary := plOptions.discardOldPassword || + (spec.AuthOpt != nil && spec.AuthOpt.AuthPlugin != "" && spec.AuthOpt.AuthPlugin != currentAuthPlugin) + hasNewAttributes := len(newAttributes) > 0 + if hasNewAttributes { + if len(newAttributes) > 1 || passwordLockingStr == "" { + passwordLockingInfo.containsNoOthers = false + } + } + switch { + case hasNewAttributes && dropSecondary && !plOptions.retainCurrentPassword: + newAttributesStr := fmt.Sprintf("{%s}", strings.Join(newAttributes, ",")) + fields = append(fields, alterField{ + "user_attributes=json_remove(json_merge_patch(coalesce(user_attributes, '{}'), %?), '$.additional_password')", + newAttributesStr, + }) + case hasNewAttributes: + newAttributesStr := fmt.Sprintf("{%s}", strings.Join(newAttributes, ",")) + fields = append(fields, alterField{"user_attributes=json_merge_patch(coalesce(user_attributes, '{}'), %?)", newAttributesStr}) + case dropSecondary && !plOptions.retainCurrentPassword: + fields = append(fields, alterField{"user_attributes=json_remove(coalesce(user_attributes, '{}'), '$.additional_password')", nil}) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/simple.go` around lines 2013 - 2041, The UPDATE can end up with two separate user_attributes assignments; instead compose them into one so we perform merge then remove in a single expression. When newAttributes (built via buildAdditionalPasswordJSON and stored in newAttributesStr) exists and dropSecondary is true (and !plOptions.retainCurrentPassword), instead of appending both alterField entries ("user_attributes=json_merge_patch(..., %?)", newAttributesStr) and ("user_attributes=json_remove(..., '$.additional_password')", nil), create a single alterField that wraps the merge with json_remove (e.g. json_remove(json_merge_patch(coalesce(user_attributes, '{}'), %?), '$.additional_password') with the same newAttributesStr) and append only that; otherwise keep the existing single-operation branches. Ensure you still set passwordLockingInfo.containsNoOthers as currently done.
🤖 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/integrationtest/t/executor/dual_password.test`:
- Line 10: The test's mysql.user checks (e.g., the query "select
json_extract(user_attributes, '$.additional_password') is not null from
mysql.user where user = 'dpu1';") are not qualified by Host and can return
multiple/flaky rows; update each such query that filters by user ('dpu1' and
'dpvictim') to also filter by host (for this test use "and host = '%'" or the
exact host you create) so the assertions target the specific account, and apply
the same change to all other similar queries in this file that reference those
users.
---
Nitpick comments:
In `@pkg/executor/simple.go`:
- Around line 2013-2024: The code performs fragile string-surgery on the JSON
returned by buildAdditionalPasswordJSON (used when
plOptions.retainCurrentPassword is true) by trimming outer braces and appending
to newAttributes; change the API so buildAdditionalPasswordJSON returns either
the inner fragment or the raw password/hash (or a structured map) and update
callers accordingly: have buildAdditionalPasswordJSON return a well-defined
fragment (or value) and then compose the surrounding JSON object at the call
site that needs it (the block that appends to newAttributes and the
executeSetPwd caller), removing any TrimPrefix/TrimSuffix operations and instead
building the JSON explicitly from the returned fragment/value to avoid brittle
string concatenation.
- Around line 1103-1110: Replace the generic errors.Errorf calls that reject
plOptions.retainCurrentPassword and plOptions.discardOldPassword with dedicated
executable errors from the exeerrors package (e.g., a new
exeerrors.ErrUnsupportedClauseForCreateUser or reuse an existing "clause not
supported in this statement" error) so the client receives proper MySQL error
codes/SQLSTATE; update the two other analogous rejection sites (the other checks
handling retainCurrentPassword/discardOldPassword elsewhere in this file) to use
the same exeerrors symbol for consistency and ensure callers return that error
value.
- Around line 2013-2041: The UPDATE can end up with two separate user_attributes
assignments; instead compose them into one so we perform merge then remove in a
single expression. When newAttributes (built via buildAdditionalPasswordJSON and
stored in newAttributesStr) exists and dropSecondary is true (and
!plOptions.retainCurrentPassword), instead of appending both alterField entries
("user_attributes=json_merge_patch(..., %?)", newAttributesStr) and
("user_attributes=json_remove(..., '$.additional_password')", nil), create a
single alterField that wraps the merge with json_remove (e.g.
json_remove(json_merge_patch(coalesce(user_attributes, '{}'), %?),
'$.additional_password') with the same newAttributesStr) and append only that;
otherwise keep the existing single-operation branches. Ensure you still set
passwordLockingInfo.containsNoOthers as currently done.
In `@tests/integrationtest/t/executor/dual_password.test`:
- Around line 41-60: Add a self-service test that ensures
APPLICATION_PASSWORD_ADMIN is not required when a user alters their own password
with RETAIN/DISCARD: use the existing dpvictim user and connect as dpvictim
(e.g., connect (cvictim, 127.0.0.1, dpvictim, v1, test)), then run alter user
dpvictim identified by 'v2' retain current password (and a DISCARD variant) and
assert success (and check mysql.user.user_attributes like the cross-user test
does); keep the cross-user tests for dpadmin unchanged so both paths
(self-service via dpvictim and cross-user via dpadmin) are covered to catch
regressions that might gate self-service operations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9c8ee980-d4fb-4123-abac-7ff348b7a7f6
📒 Files selected for processing (16)
pkg/errno/errcode.gopkg/errno/errname.gopkg/executor/simple.gopkg/executor/test/passwordtest/BUILD.bazelpkg/executor/test/passwordtest/dual_password_test.gopkg/parser/ast/misc.gopkg/parser/keywords.gopkg/parser/misc.gopkg/parser/parser.gopkg/parser/parser.ypkg/parser/parser_test.gopkg/privilege/privileges/cache.gopkg/privilege/privileges/privileges.gopkg/util/dbterror/exeerrors/errors.gotests/integrationtest/r/executor/dual_password.resulttests/integrationtest/t/executor/dual_password.test
✅ Files skipped from review due to trivial changes (4)
- pkg/parser/keywords.go
- pkg/parser/misc.go
- pkg/parser/parser.y
- pkg/parser/parser_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/executor/test/passwordtest/BUILD.bazel
- pkg/executor/test/passwordtest/dual_password_test.go
Contributor
Author
|
improved 8.0 compatibility |
Contributor
Author
MySQL upstream test-coverage analysisCross-checked this PR's tests against MySQL's own dual-password suite so reviewers can see exactly which scenarios are covered and which are intentionally deferred. MySQL's test files (github.com/mysql/mysql-server)
Coverage matrix
Score: 8 of 14 scenarios covered. Additional TiDB-specific coverage not in the MySQL suite: Gaps I'd recommend closing before final review
Deliberately skipped
Happy to add #3–#7 as a follow-up commit if reviewers agree they're worth the marginal test cost. |
ti-chi-bot
Bot
added
the
do-not-merge/work-in-progress
…D PASSWORD) Adds MySQL 8.0-compatible dual-password support for credential rotation without downtime: - Parser: new RETAIN CURRENT PASSWORD and DISCARD OLD PASSWORD clauses on ALTER USER and SET PASSWORD; RETAIN/DISCARD/OLD registered as non-reserved keywords. - Privilege cache: UserRecord.AdditionalAuthenticationString, decoded from mysql.user.user_attributes '$.additional_password'. - Auth: ConnectionVerification falls back to the secondary password for mysql_native_password / caching_sha2_password / tidb_sm3_password when the primary check fails. - Privilege: APPLICATION_PASSWORD_ADMIN dynamic privilege; required when RETAIN/DISCARD is applied to another user's account (self-service is always allowed, whether the statement uses CURRENT_USER() or names the caller explicitly). - Executor: executeCreateUser rejects RETAIN/DISCARD (per MySQL); executeAlterUser and executeSetPwd promote the current authentication_string to user_attributes.additional_password on RETAIN, JSON_REMOVE on DISCARD, and silently drop the secondary on a plugin change (matching MySQL behavior). - Error codes: ErrCurrentPasswordCannotBeRetainedPluginChange (4058), ErrCurrentPasswordCannotBeRetainedEmptyNew (4059) — MySQL-compatible. - Tests: 10 unit tests in pkg/executor/test/passwordtest/dual_password_test.go plus integration test in tests/integrationtest/t/executor/dual_password.test. Ref pingcap#60587
takaidohigasi
force-pushed
the
feature/dual-password
branch
from
7386798 to
1c659b0
Compare
Contributor
Author
Coverage update — MySQL scenarios #3–#7 added (commit
|
| # | MySQL scenario | New test |
|---|---|---|
| 3 | Chained RETAIN — secondary becomes previous primary each time |
TestDualPasswordChainedRetain |
| 4 | ALTER USER without RETAIN preserves existing secondary |
TestDualPasswordAlterWithoutRetainPreservesSecondary |
| 5 | RENAME USER preserves user_attributes.$.additional_password |
TestDualPasswordRenameUserPreservesSecondary |
| 6 | DROP USER removes secondary (row gone) |
TestDualPasswordDropUserRemovesSecondary |
| 7 | Multi-user ALTER USER with trailing RETAIN / DISCARD |
TestDualPasswordMultiUserAlter |
New coverage score: 13 of 14 MySQL scenarios (up from 8 of 14). Remaining deliberate gaps: non-built-in plugin (#12), password-expiry interaction (#13), binary log (#14) — each justified in the earlier analysis.
Caveat on scenario #7
MySQL allows per-spec RETAIN / DISCARD inside a multi-user ALTER USER:
ALTER USER u1 IDENTIFIED BY 'a' RETAIN CURRENT PASSWORD,
u2 IDENTIFIED BY 'b',
u3 DISCARD OLD PASSWORD;TiDB's grammar currently attaches PasswordOrLockOptions at the statement level (shared across all user specs), so the trailing clause applies to every spec. The test pins this behavior and the docstring flags the gap; tightening to MySQL's per-spec grammar would be a larger change worth a follow-up PR if reviewers prefer.
Verification
go test -tags=intest -count=1 -v -run '^TestDualPassword' ./pkg/executor/test/passwordtest/— 16 / 16 PASS (up from 11).gofmt -l— clean;make lint— clean.make bazel_prepare— no metadata drift beyond the expectedshard_countbump.bazel build --config=ci //... --//build:with_nogo_flag=true --//build:with_rbe_flag=true— 1406 targets, exit 0.
PR branch: feature/dual-password @ 1c659b0ac.
Contributor
|
I've update the description like this: Tests <!-- At least one of them must be included. -->The comment here was missing and this is required for the check to work. |
Contributor
Author
|
thanks, It was my problem. |
D3Hunter
reviewed
May 14, 2026
Contributor
Author
|
thanks I got it! |
Per @D3Hunter's review request to split this PR for easier review, back out everything except the parser surface. The behavior side (executor, privilege, error codes, dual-password tests, integration test) lives on `feature/dual-password-behavior` and will be opened as a separate PR stacked on this one. Reverts on this branch: - pkg/errno/{errcode,errname}.go → restored from master - pkg/util/dbterror/exeerrors/errors.go → restored from master - pkg/executor/simple.go → restored from master - pkg/privilege/privileges/{cache,privileges}.go → restored from master - pkg/executor/test/passwordtest/BUILD.bazel → restored from master - pkg/executor/test/passwordtest/dual_password_test.go → deleted - tests/integrationtest/r/executor/executor.result → restored from master - tests/integrationtest/t/executor/dual_password.test → deleted - tests/integrationtest/r/executor/dual_password.result → deleted - errors.toml → restored from master Kept in this PR (parser surface only): - pkg/parser/parser.y + parser.go (regenerated) + keywords.go - pkg/parser/misc.go (RETAIN/DISCARD/OLD keyword map) - pkg/parser/ast/misc.go (PasswordOrLockOption RETAIN/DISCARD constants, UserSpec.DualPasswordOption, SetPwdStmt.RetainCurrentPassword, Restore / SecurityString / SecureText updates) - pkg/parser/parser_test.go + keywords_test.go (parser round-trip) Verified: - pkg/parser/... unit tests: PASS - go build ./pkg/executor/... ./pkg/privilege/... ./pkg/errno/... ./pkg/util/dbterror/...: clean (no executor/privilege code references the new parser AST symbols on this branch) - bazel build --config=ci //... --//build:with_nogo_flag=true --//build:with_rbe_flag=true: 1406 targets, exit 0 Ref pingcap#60587
takaidohigasi
changed the title
ti-chi-bot
Bot
added
do-not-merge/needs-tests-checked
release-note-none
13 tasks
Contributor
Author
|
@D3Hunter thanks for the suggestion — agree it makes review much easier. Done:
Both PRs preserve the template HTML comments (including Will ping again once CI on the parser-only tip is green. |
Contributor
Author
|
/retest |
Contributor
Author
|
/retest-required |
1 similar comment
Contributor
Author
|
/retest-required |
D3Hunter
reviewed
May 15, 2026
Contributor
D3Hunter
left a comment
D3Hunter
left a comment
There was a problem hiding this comment.
Summary
- Total findings: 8
- Inline comments: 8
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
🚨 [Blocker] (1)
- Dual-password clauses are parsed but ignored by execution, causing silent semantic breakage (
pkg/parser/parser.y:11287; pkg/parser/parser.y:14486; pkg/parser/parser.y:14539; pkg/parser/ast/misc.go:1405; pkg/parser/ast/misc.go:1552; pkg/executor/simple.go:1818; pkg/executor/simple.go:2495; pkg/executor/simple.go:2602)
⚠️ [Major] (4)
- CREATE USER accepts dual-password clauses outside MySQL grammar and ignores them at runtime (
pkg/parser/parser.y:14373; pkg/parser/parser.y:14486; pkg/parser/parser.y:14539; pkg/parser/parser.y:14840; pkg/executor/simple.go:1158) - ALTER USER over-accepts invalid RETAIN combinations (including hashed AS form and no-auth form) (
pkg/parser/parser.y:14486; pkg/parser/parser.y:14518; pkg/parser/parser.y:14539; pkg/parser/parser_test.go:5310) - Grammar label/documentation contradicts actual dual-password acceptance (
pkg/parser/parser.y:1464; pkg/parser/parser.y:14486; pkg/parser/parser_test.go:5306) - Dual-password state is represented by a generic password/lock option type (
pkg/parser/ast/misc.go:1552; pkg/parser/ast/misc.go:1569; pkg/parser/ast/misc.go:1725; pkg/parser/ast/misc.go:1770; pkg/parser/parser.y:14486)
🟡 [Minor] (3)
- ALTER USER USER() branch lacks MySQL user_func_auth_option dual-password forms (
pkg/parser/parser.y:14406; pkg/parser/parser.y:14426) - Dual-password parser coverage lacks negative/boundary assertions and rationale for controversial accepted forms (
pkg/parser/parser_test.go:5306; pkg/parser/parser_test.go:5310; pkg/parser/parser_test.go:5318) - Duplicated CREATE/ALTER user-spec grammar chains increase drift risk (
pkg/parser/parser.y:14486; pkg/parser/parser.y:14504; pkg/parser/parser.y:14518; pkg/parser/parser.y:14529)
| Username AuthOption UserSpecDualPasswordOption | ||
| { | ||
| userSpec := &ast.UserSpec{ | ||
| User: $1.(*auth.UserIdentity), |
Contributor
There was a problem hiding this comment.
⚠️ [Major] ALTER USER over-accepts invalid RETAIN combinations (including hashed AS form and no-auth form)
Why
UserSpecDualPasswordOption is parsed independently from optional AuthOption, so RETAIN CURRENT PASSWORD can be accepted for auth alternatives where MySQL does not allow it, and even when no password-change clause is present.
Scope
pkg/parser/parser.y:14486; pkg/parser/parser.y:14518; pkg/parser/parser.y:14539; pkg/parser/parser_test.go:5310
Risk if unchanged
Invalid SQL can be normalized as valid, hiding application bugs and diverging from MySQL behavior.
Evidence
AlterUserSpec accepts Username AuthOption UserSpecDualPasswordOption with optional auth, and parser tests include IDENTIFIED WITH ... AS '<hash>' RETAIN CURRENT PASSWORD as valid.
Change request
Constrain grammar so RETAIN is accepted only with supported password-changing auth alternatives; reject ... AS <hash> RETAIN CURRENT PASSWORD and no-auth RETAIN forms. Add regression tests for these invalid combinations.
Contributor
Author
There was a problem hiding this comment.
Fixed in adb27a700. Introduced AuthOptionWithPassword (the BY-form subset of AuthOption) and restructured AlterUserSpec into three variants so RETAIN CURRENT PASSWORD only binds to the BY-form auth. The following are now parser-level errors:
ALTER USER u IDENTIFIED WITH plugin AS '<hash>' RETAIN CURRENT PASSWORDALTER USER u IDENTIFIED WITH plugin RETAIN CURRENT PASSWORDALTER USER u RETAIN CURRENT PASSWORD
Pinned by negative cases at parser_test.go:5323, :5325, :5328.
Fixes for D3Hunter's review on pingcap#68028: [Blocker] Dual-password clauses are parsed but ignored by execution - Add executor stubs that reject RETAIN CURRENT PASSWORD / DISCARD OLD PASSWORD with ER_NOT_SUPPORTED_YET in executeAlterUser and executeSetPwd. Full execution / privilege / storage logic lives in the follow-up PR pingcap#68393. - Expose exeerrors.ErrNotSupportedYet for callers (was previously only available via dbterror.ClassExecutor.NewStd inline). - New TestDualPasswordParserOnlyStub in passwordtest to pin the stub's behavior. [Major pingcap#2] CREATE USER accepted dual-password clauses outside MySQL grammar - Drop the CreateUserSpec/CreateUserSpecList non-terminals and route CREATE USER back through the original UserSpec/UserSpecList chain. CREATE USER + RETAIN/DISCARD is now a parser-level syntax error, matching MySQL. [Major pingcap#3] ALTER USER over-accepted invalid RETAIN combinations - Introduce AuthOptionWithPassword (the BY-form subset of AuthOption) and restructure AlterUserSpec to bind RETAIN only to that subset. Reject at parse time: ALTER USER u IDENTIFIED WITH plugin AS '<hash>' RETAIN ... ALTER USER u IDENTIFIED WITH plugin RETAIN ... ALTER USER u RETAIN CURRENT PASSWORD (no auth) ALTER USER u IDENTIFIED BY '...' DISCARD OLD PASSWORD - DISCARD becomes its own AlterUserSpec variant (no auth-option coexists with it). [Major pingcap#4] Grammar label/documentation contradiction - Drop the misleading 'unsupported dual password option' label on the deleted CreateUserSpec; rewrite UserSpec/AlterUserSpec labels to state the contract accurately. [Major pingcap#5] Generic *PasswordOrLockOption type was overloaded - Introduce dedicated ast.DualPasswordOption and ast.DualPasswordOptionType (with DualPasswordRetainCurrent / DualPasswordDiscardOld). UserSpec.DualPasswordOption now uses the dedicated type. Remove the RetainCurrentPassword and DiscardOldPassword iota entries from PasswordOrLockOption. [Minor pingcap#6] ALTER USER USER() branch - USER() still does not route through AlterUserSpecList; dual-password on USER() is now a parse-time syntax error (covered by negative test). [Minor pingcap#7] Parser tests had no negative coverage - Add explicit ok=false rows for CREATE USER + RETAIN, the AS-hash + RETAIN form, bare RETAIN with no auth, plain BY + DISCARD, bare-plugin + RETAIN, and ALTER USER USER() + RETAIN. [Minor pingcap#8] CREATE/ALTER user-spec grammar duplication - Now naturally eliminated because CREATE USER reuses UserSpec. Behavior PR (pingcap#68393) needs to: - Remove the executor stubs added here. - Adopt the new ast.DualPasswordOption / DualPasswordOptionType symbols (the iota entries it currently consumes are gone). Ref pingcap#60587
MySQL 8.0 `user_func_auth_option` allows dual-password clauses on the current-user form. The earlier parser PR (pingcap#68028) rejected them at parse time, which would have bypassed the executor stub for clients that emit the `ALTER USER USER() ...` shape. Adds two grammar productions for the USER() branch: ALTER USER [IF EXISTS] USER() IDENTIFIED BY '...' RETAIN CURRENT PASSWORD ALTER USER [IF EXISTS] USER() DISCARD OLD PASSWORD These attach the dual-password clause to a new `AlterUserStmt.CurrentDualPasswordOption` field; the executor stub checks it alongside the per-spec field and returns ER_NOT_SUPPORTED_YET. Restore() round-trips both forms. Stub test extended with USER() coverage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Codex review (GPT-5.5, high effort) flagged one P2 on Fix in
Validation: parser |
Fix `check_dev` errdoc failure. The earlier review-fixup commit added `exeerrors.ErrNotSupportedYet = dbterror.ClassExecutor.NewStd(...)`, which duplicates `ErrFuncNotEnabled` already registered under `executor:1235`. `errdoc` then picked the new registration's message and the regenerated `errors.toml` no longer matched the checked-in copy. Drop the new registration and route the three stub call sites through the canonical `plannererrors.ErrNotSupportedYet` (already imported in `simple.go`, same MySQL error 1235 / message). No on-wire behavior change; tests continue to expect `errno.ErrNotSupportedYet`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
/retest-required |
D3Hunter
approved these changes
May 18, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: D3Hunter 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 |
[LGTM Timeline notifier]Timeline:
|
Contributor
Author
|
@BornChanger I am glad if you take a look if you have time. thanks! |
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
4 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.
What problem does this PR solve?
Issue Number: ref #60587
Problem Summary:
This PR is the parser-only half of the MySQL 8.0 dual-password feature, split from the original PR per @D3Hunter's review request for easier review.
The behavior side (executor, privilege, error codes, dual-password tests, integration test) lives in a follow-up PR which stacks on top of this one: #68393.
What changed and how does it work?
Parser surface only:
pkg/parser/parser.yRETAIN CURRENT PASSWORDandDISCARD OLD PASSWORDperUserSpec(per-spec, matching MySQL);SET PASSWORD ... RETAIN CURRENT PASSWORD. New non-reserved keywordsRETAIN,OLD(DISCARD,CURRENT,PASSWORDalready existed).pkg/parser/parser.go,pkg/parser/keywords.goparser.yviagoyacc(large mechanical diff).pkg/parser/misc.goOLD,RETAINto keyword map.pkg/parser/ast/misc.goRetainCurrentPassword,DiscardOldPasswordin thePasswordOrLockOptioniota;UserSpec.DualPasswordOptionfield;SetPwdStmt.RetainCurrentPasswordfield;Restorewrites the clauses;SecurityStringandSecureTextsurface the (non-secret) clauses for redacted output.pkg/parser/parser_test.goIDENTIFIED WITH plugin).pkg/parser/keywords_test.goTestKeywordsLengthcount bumped 679 → 681 (two new non-reserved keywords; reserved count unchanged).The
parser.goregenerated diff is ~11k lines but is purely goyacc table output. Hand-written changes are ~70 lines acrossparser.y,ast/misc.go,misc.go, and the tests.Check List
Tests
pkg/parser/parser_test.go— parse + restore round-trip for all RETAIN/DISCARD forms (ALTER USER, SET PASSWORD).cd pkg/parser && go test -tags=intest ./...— all subpackages pass.go build ./pkg/executor/... ./pkg/privilege/... ./pkg/errno/... ./pkg/util/dbterror/...— clean compile (no other package depends on the new AST symbols on this branch).bazel build --config=ci //... --//build:with_nogo_flag=true --//build:with_rbe_flag=true— 1406 targets, exit 0.Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.