Skip to content

Commit adb27a7

Browse files
committed
*: address @D3Hunter review (parser PR)
Fixes for D3Hunter's review on #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 #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 #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 #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 #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 #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 #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 #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 #8] CREATE/ALTER user-spec grammar duplication - Now naturally eliminated because CREATE USER reuses UserSpec. Behavior PR (#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 #60587
1 parent 8776699 commit adb27a7

8 files changed

Lines changed: 7171 additions & 7083 deletions

File tree

pkg/executor/simple.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1649,6 +1649,16 @@ func checkPasswordReusePolicy(ctx context.Context, sqlExecutor sqlexec.SQLExecut
16491649
}
16501650

16511651
func (e *SimpleExec) executeAlterUser(ctx context.Context, s *ast.AlterUserStmt) error {
1652+
// MySQL 8.0 dual-password clauses (RETAIN CURRENT PASSWORD /
1653+
// DISCARD OLD PASSWORD) are accepted by the parser in this PR but the
1654+
// matching executor / privilege / storage logic lands in a follow-up
1655+
// (pingcap/tidb#68393). Reject explicitly with a stable error so users
1656+
// see "not supported yet" instead of silent success.
1657+
for _, spec := range s.Specs {
1658+
if spec.DualPasswordOption != nil {
1659+
return exeerrors.ErrNotSupportedYet.GenWithStackByArgs("dual password (RETAIN CURRENT PASSWORD / DISCARD OLD PASSWORD)")
1660+
}
1661+
}
16521662
disableSandBoxMode := false
16531663
var err error
16541664
if e.Ctx().InSandBoxMode() {
@@ -2493,6 +2503,11 @@ func userExistsInternal(ctx context.Context, sqlExecutor sqlexec.SQLExecutor, na
24932503
}
24942504

24952505
func (e *SimpleExec) executeSetPwd(ctx context.Context, s *ast.SetPwdStmt) error {
2506+
// See executeAlterUser: RETAIN CURRENT PASSWORD parsing lands in this PR;
2507+
// execution lands in pingcap/tidb#68393. Until then, fail closed.
2508+
if s.RetainCurrentPassword {
2509+
return exeerrors.ErrNotSupportedYet.GenWithStackByArgs("SET PASSWORD ... RETAIN CURRENT PASSWORD")
2510+
}
24962511
ctx = kv.WithInternalSourceType(ctx, kv.InternalTxnPrivilege)
24972512
sysSession, err := e.GetSysSession()
24982513
if err != nil {

pkg/executor/test/passwordtest/BUILD.bazel

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ go_test(
88
"password_management_test.go",
99
],
1010
flaky = True,
11-
shard_count = 9,
11+
shard_count = 10,
1212
deps = [
1313
"//pkg/domain",
1414
"//pkg/errno",

pkg/executor/test/passwordtest/password_management_test.go

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -989,6 +989,35 @@ func checkAuthUser(t *testing.T, tk *testkit.TestKit, user string, failedLoginCo
989989
require.Equal(t, autoAccountLocked, ua[0].PasswordLocking.AutoAccountLocked)
990990
}
991991

992+
// TestDualPasswordParserOnlyStub guards the executor stub added in the
993+
// parser-only PR (#68028). The grammar now accepts MySQL 8.0 dual-password
994+
// clauses (RETAIN CURRENT PASSWORD / DISCARD OLD PASSWORD), but the matching
995+
// executor / privilege / storage logic lands in the follow-up PR (#68393).
996+
// Until then the executor must fail fast with ER_NOT_SUPPORTED_YET so users
997+
// don't see silent success.
998+
//
999+
// When #68393 lands and removes the stubs, this test should be replaced by
1000+
// the real dual-password coverage that lives there.
1001+
func TestDualPasswordParserOnlyStub(t *testing.T) {
1002+
store := testkit.CreateMockStore(t)
1003+
tk := testkit.NewTestKit(t, store)
1004+
require.NoError(t, tk.Session().Auth(&auth.UserIdentity{Username: "root", Hostname: "%"}, nil, nil, nil))
1005+
1006+
tk.MustExec("DROP USER IF EXISTS dpstub")
1007+
tk.MustExec("CREATE USER dpstub IDENTIFIED BY 'old'")
1008+
1009+
// ALTER USER ... RETAIN CURRENT PASSWORD must fail with ER_NOT_SUPPORTED_YET.
1010+
tk.MustGetErrCode("ALTER USER dpstub IDENTIFIED BY 'new' RETAIN CURRENT PASSWORD", errno.ErrNotSupportedYet)
1011+
// ALTER USER ... DISCARD OLD PASSWORD must fail with ER_NOT_SUPPORTED_YET.
1012+
tk.MustGetErrCode("ALTER USER dpstub DISCARD OLD PASSWORD", errno.ErrNotSupportedYet)
1013+
// SET PASSWORD ... RETAIN CURRENT PASSWORD must fail with ER_NOT_SUPPORTED_YET.
1014+
tk.MustGetErrCode("SET PASSWORD FOR dpstub = 'new' RETAIN CURRENT PASSWORD", errno.ErrNotSupportedYet)
1015+
1016+
// A regular ALTER USER (no dual-password clause) must still succeed —
1017+
// the stub guard only triggers when DualPasswordOption is set.
1018+
tk.MustExec("ALTER USER dpstub IDENTIFIED BY 'plain'")
1019+
}
1020+
9921021
func selectSQL(user string) string {
9931022
userAttributesSQL := new(strings.Builder)
9941023
sqlescape.MustFormatSQL(userAttributesSQL, "SELECT user_attributes from mysql.user WHERE USER = %? AND HOST = 'localhost' for update", user)

pkg/parser/ast/misc.go

Lines changed: 36 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1554,7 +1554,7 @@ func (n *SetDefaultRoleStmt) Accept(v Visitor) (Node, bool) {
15541554
type UserSpec struct {
15551555
User *auth.UserIdentity
15561556
AuthOpt *AuthOption
1557-
DualPasswordOption *PasswordOrLockOption
1557+
DualPasswordOption *DualPasswordOption
15581558
IsRole bool
15591559
}
15601560

@@ -1592,9 +1592,9 @@ func (n *UserSpec) SecurityString() string {
15921592
dualClause := ""
15931593
if n.DualPasswordOption != nil {
15941594
switch n.DualPasswordOption.Type {
1595-
case RetainCurrentPassword:
1595+
case DualPasswordRetainCurrent:
15961596
dualClause = " RETAIN CURRENT PASSWORD"
1597-
case DiscardOldPassword:
1597+
case DualPasswordDiscardOld:
15981598
dualClause = " DISCARD OLD PASSWORD"
15991599
}
16001600
}
@@ -1725,11 +1725,42 @@ const (
17251725
PasswordRequireCurrentDefault
17261726

17271727
UserResourceGroupName
1728+
)
1729+
1730+
// DualPasswordOptionType identifies the dual-password action attached to a
1731+
// UserSpec by the parser. Dedicated to dual-password semantics so the AST
1732+
// does not conflate it with PasswordOrLockOption (account lock, expiration,
1733+
// failed-login policy, etc.) which has different per-statement scoping rules.
1734+
type DualPasswordOptionType int
17281735

1729-
RetainCurrentPassword
1730-
DiscardOldPassword
1736+
const (
1737+
// DualPasswordRetainCurrent corresponds to RETAIN CURRENT PASSWORD.
1738+
DualPasswordRetainCurrent DualPasswordOptionType = iota + 1
1739+
// DualPasswordDiscardOld corresponds to DISCARD OLD PASSWORD.
1740+
DualPasswordDiscardOld
17311741
)
17321742

1743+
// DualPasswordOption represents a per-UserSpec MySQL 8.0 dual-password clause
1744+
// (RETAIN CURRENT PASSWORD or DISCARD OLD PASSWORD). The grammar attaches it
1745+
// to the UserSpec rather than to AlterUserStmt because MySQL allows different
1746+
// dual-password actions per spec inside a multi-user ALTER USER statement.
1747+
type DualPasswordOption struct {
1748+
Type DualPasswordOptionType
1749+
}
1750+
1751+
// Restore implements Node interface.
1752+
func (d *DualPasswordOption) Restore(ctx *format.RestoreCtx) error {
1753+
switch d.Type {
1754+
case DualPasswordRetainCurrent:
1755+
ctx.WriteKeyWord("RETAIN CURRENT PASSWORD")
1756+
case DualPasswordDiscardOld:
1757+
ctx.WriteKeyWord("DISCARD OLD PASSWORD")
1758+
default:
1759+
return errors.Errorf("Unsupported DualPasswordOption.Type %d", d.Type)
1760+
}
1761+
return nil
1762+
}
1763+
17331764
type PasswordOrLockOption struct {
17341765
Type int
17351766
Count int64
@@ -1770,10 +1801,6 @@ func (p *PasswordOrLockOption) Restore(ctx *format.RestoreCtx) error {
17701801
ctx.WriteKeyWord(" DAY")
17711802
case PasswordReuseDefault:
17721803
ctx.WriteKeyWord("PASSWORD REUSE INTERVAL DEFAULT")
1773-
case RetainCurrentPassword:
1774-
ctx.WriteKeyWord("RETAIN CURRENT PASSWORD")
1775-
case DiscardOldPassword:
1776-
ctx.WriteKeyWord("DISCARD OLD PASSWORD")
17771804
default:
17781805
return errors.Errorf("Unsupported PasswordOrLockOption.Type %d", p.Type)
17791806
}

0 commit comments

Comments
 (0)