From c8ba4e3ace24bebcfc1b7008acc4e337ecb4c1e0 Mon Sep 17 00:00:00 2001 From: tiancaiamao Date: Tue, 17 Dec 2024 11:33:42 +0800 Subject: [PATCH] *: avoid some unnecessary call of ensureActiveUser() (#57388) ref pingcap/tidb#55563 --- pkg/executor/show.go | 5 +++-- pkg/executor/simple.go | 19 +++++-------------- pkg/extension/auth_test.go | 2 +- pkg/privilege/privilege.go | 3 --- pkg/privilege/privileges/privileges.go | 16 ---------------- pkg/privilege/privileges/privileges_test.go | 11 ++++++----- .../integrationtest/r/executor/simple.result | 3 +++ tests/integrationtest/t/executor/simple.test | 3 ++- 8 files changed, 20 insertions(+), 42 deletions(-) diff --git a/pkg/executor/show.go b/pkg/executor/show.go index 35826630ef..bad01f3305 100644 --- a/pkg/executor/show.go +++ b/pkg/executor/show.go @@ -1751,7 +1751,7 @@ func (e *ShowExec) fetchShowCreateUser(ctx context.Context) error { `SELECT plugin, Account_locked, user_attributes->>'$.metadata', Token_issuer, Password_reuse_history, Password_reuse_time, Password_expired, Password_lifetime, user_attributes->>'$.Password_locking.failed_login_attempts', - user_attributes->>'$.Password_locking.password_lock_time_days' + user_attributes->>'$.Password_locking.password_lock_time_days', authentication_string FROM %n.%n WHERE User=%? AND Host=%?`, mysql.SystemDB, mysql.UserTable, userName, strings.ToLower(hostName)) if err != nil { @@ -1829,6 +1829,8 @@ func (e *ShowExec) fetchShowCreateUser(ctx context.Context) error { passwordLockTimeDays = " PASSWORD_LOCK_TIME " + passwordLockTimeDays } } + authData := rows[0].GetString(10) + rows, _, err = exec.ExecRestrictedSQL(ctx, nil, `SELECT Priv FROM %n.%n WHERE User=%? AND Host=%?`, mysql.SystemDB, mysql.GlobalPrivTable, userName, hostName) if err != nil { return errors.Trace(err) @@ -1845,7 +1847,6 @@ func (e *ShowExec) fetchShowCreateUser(ctx context.Context) error { require = privValue.RequireStr() } - authData := checker.GetEncodedPassword(ctx, e.User.Username, e.User.Hostname) authStr := "" if !(authPlugin == mysql.AuthSocket && authData == "") { authStr = fmt.Sprintf(" AS '%s'", authData) diff --git a/pkg/executor/simple.go b/pkg/executor/simple.go index 3f9b17fdf5..76b7799ed1 100644 --- a/pkg/executor/simple.go +++ b/pkg/executor/simple.go @@ -1778,15 +1778,15 @@ func (e *SimpleExec) executeAlterUser(ctx context.Context, s *ast.AlterUserStmt) if !(hasCreateUserPriv || hasSystemSchemaPriv) { return plannererrors.ErrSpecificAccessDenied.GenWithStackByArgs("CREATE USER") } - if checker.RequestDynamicVerificationWithUser(ctx, "SYSTEM_USER", false, spec.User) && !(hasSystemUserPriv || hasRestrictedUserPriv) { + if !(hasSystemUserPriv || hasRestrictedUserPriv) && checker.RequestDynamicVerificationWithUser(ctx, "SYSTEM_USER", false, spec.User) { return plannererrors.ErrSpecificAccessDenied.GenWithStackByArgs("SYSTEM_USER or SUPER") } - if sem.IsEnabled() && checker.RequestDynamicVerificationWithUser(ctx, "RESTRICTED_USER_ADMIN", false, spec.User) && !hasRestrictedUserPriv { + if sem.IsEnabled() && !hasRestrictedUserPriv && checker.RequestDynamicVerificationWithUser(ctx, "RESTRICTED_USER_ADMIN", false, spec.User) { return plannererrors.ErrSpecificAccessDenied.GenWithStackByArgs("RESTRICTED_USER_ADMIN") } } - exists, _, err := userExistsInternal(ctx, sqlExecutor, spec.User.Username, spec.User.Hostname) + exists, currentAuthPlugin, err := userExistsInternal(ctx, sqlExecutor, spec.User.Username, spec.User.Hostname) if err != nil { return err } @@ -1795,10 +1795,6 @@ func (e *SimpleExec) executeAlterUser(ctx context.Context, s *ast.AlterUserStmt) failedUsers = append(failedUsers, user) continue } - currentAuthPlugin, err := privilege.GetPrivilegeManager(e.Ctx()).GetAuthPlugin(ctx, spec.User.Username, spec.User.Hostname) - if err != nil { - return err - } type AuthTokenOptionHandler int const ( @@ -2311,7 +2307,7 @@ func (e *SimpleExec) executeDropUser(ctx context.Context, s *ast.DropUserStmt) e // Because in TiDB SUPER can be used as a substitute for any dynamic privilege, this effectively means that // any user with SUPER requires a user with SUPER to be able to DROP the user. // We also allow RESTRICTED_USER_ADMIN to count for simplicity. - if checker.RequestDynamicVerificationWithUser(ctx, "SYSTEM_USER", false, user) && !(hasSystemUserPriv || hasRestrictedUserPriv) { + if !(hasSystemUserPriv || hasRestrictedUserPriv) && checker.RequestDynamicVerificationWithUser(ctx, "SYSTEM_USER", false, user) { if _, err := sqlExecutor.ExecuteInternal(internalCtx, "rollback"); err != nil { return err } @@ -2522,14 +2518,13 @@ func (e *SimpleExec) executeSetPwd(ctx context.Context, s *ast.SetPwdStmt) error h = s.User.Hostname checker := privilege.GetPrivilegeManager(e.Ctx()) - checker.MatchIdentity(ctx, u, h, false) activeRoles := e.Ctx().GetSessionVars().ActiveRoles if checker != nil && !checker.RequestVerification(activeRoles, "", "", "", mysql.SuperPriv) { currUser := e.Ctx().GetSessionVars().User return exeerrors.ErrDBaccessDenied.GenWithStackByArgs(currUser.Username, currUser.Hostname, "mysql") } } - exists, _, err := userExistsInternal(ctx, sqlExecutor, u, h) + exists, authplugin, err := userExistsInternal(ctx, sqlExecutor, u, h) if err != nil { return err } @@ -2544,10 +2539,6 @@ func (e *SimpleExec) executeSetPwd(ctx context.Context, s *ast.SetPwdStmt) error disableSandboxMode = true } - authplugin, err := privilege.GetPrivilegeManager(e.Ctx()).GetAuthPlugin(ctx, u, h) - if err != nil { - return err - } if e.isValidatePasswordEnabled() { if err := pwdValidator.ValidatePassword(e.Ctx().GetSessionVars(), s.Password); err != nil { return err diff --git a/pkg/extension/auth_test.go b/pkg/extension/auth_test.go index 1aa2917ba6..4098eae819 100644 --- a/pkg/extension/auth_test.go +++ b/pkg/extension/auth_test.go @@ -235,7 +235,7 @@ func TestAuthPlugin(t *testing.T) { // Should authenticate using plugin impl. p.AssertNumberOfCalls(t, "AuthenticateUser", 2) p.AssertCalled(t, "ValidateAuthString", "encodedpassword") - p.AssertNumberOfCalls(t, "ValidateAuthString", 4) + p.AssertNumberOfCalls(t, "ValidateAuthString", 3) // Change password should work using ALTER USER statement. tk.MustExec("alter user 'u2'@'localhost' identified with 'authentication_test_plugin' by 'anotherrawpassword'") diff --git a/pkg/privilege/privilege.go b/pkg/privilege/privilege.go index ce2fcea5f7..f421c3f657 100644 --- a/pkg/privilege/privilege.go +++ b/pkg/privilege/privilege.go @@ -48,9 +48,6 @@ type Manager interface { // ShowGrants shows granted privileges for user. ShowGrants(ctx context.Context, sctx sessionctx.Context, user *auth.UserIdentity, roles []*auth.RoleIdentity) ([]string, error) - // GetEncodedPassword shows the encoded password for user. - GetEncodedPassword(ctx context.Context, user, host string) string - // RequestVerification verifies user privilege for the request. // If table is "", only check global/db scope privileges. // If table is not "", check global/db/table scope privileges. diff --git a/pkg/privilege/privileges/privileges.go b/pkg/privilege/privileges/privileges.go index 6f255ba185..0145aa5263 100644 --- a/pkg/privilege/privileges/privileges.go +++ b/pkg/privilege/privileges/privileges.go @@ -317,22 +317,6 @@ func (p *UserPrivileges) isValidHash(record *UserRecord) bool { return false } -// GetEncodedPassword implements the Manager interface. -func (p *UserPrivileges) GetEncodedPassword(ctx context.Context, user, host string) string { - terror.Log(p.Handle.ensureActiveUser(ctx, user)) - mysqlPriv := p.Handle.Get() - record := mysqlPriv.connectionVerification(user, host) - if record == nil { - logutil.BgLogger().Error("get user privilege record fail", - zap.String("user", user), zap.String("host", host)) - return "" - } - if p.isValidHash(record) { - return record.AuthenticationString - } - return "" -} - // GetAuthPluginForConnection gets the authentication plugin used in connection establishment. func (p *UserPrivileges) GetAuthPluginForConnection(ctx context.Context, user, host string) (string, error) { if SkipWithGrant { diff --git a/pkg/privilege/privileges/privileges_test.go b/pkg/privilege/privileges/privileges_test.go index 9fa0babdb6..7a0da0a20d 100644 --- a/pkg/privilege/privileges/privileges_test.go +++ b/pkg/privilege/privileges/privileges_test.go @@ -2091,6 +2091,7 @@ func TestNilHandleInConnectionVerification(t *testing.T) { func testShowGrantsSQLMode(t *testing.T, tk *testkit.TestKit, expected []string) { pc := privilege.GetPrivilegeManager(tk.Session()) + pc.MatchIdentity(context.Background(), "show_sql_mode", "localhost", false) gs, err := pc.ShowGrants(context.Background(), tk.Session(), &auth.UserIdentity{Username: "show_sql_mode", Hostname: "localhost"}, nil) require.NoError(t, err) require.Len(t, gs, 2) @@ -2127,12 +2128,12 @@ func TestEnsureActiveUserCoverage(t *testing.T) { sql string visited bool }{ - // FIXME {"drop user if exists 'test1'", false}, - // FIXME {"alter user test identified by 'test1'", false}, - // {"set password for test = 'test2'", false}, - // FIXME {"show create user test", false}, + {"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}, - // FIXME {"show grants", false}, + {"show grants", true}, {"show grants for 'test'@'%'", true}, } diff --git a/tests/integrationtest/r/executor/simple.result b/tests/integrationtest/r/executor/simple.result index ca40620c61..783c513271 100644 --- a/tests/integrationtest/r/executor/simple.result +++ b/tests/integrationtest/r/executor/simple.result @@ -153,6 +153,9 @@ create user test_all; set default role all to test_all; drop user if exists 'testflush'@'localhost'; CREATE USER 'testflush'@'localhost' IDENTIFIED BY ''; +SHOW GRANTS FOR 'testflush'@'localhost'; +Grants for testflush@localhost +GRANT USAGE ON *.* TO 'testflush'@'localhost' UPDATE mysql.User SET Select_priv='Y' WHERE User="testflush" and Host="localhost"; SELECT authentication_string FROM mysql.User WHERE User="testflush" and Host="localhost"; Error 1142 (42000): SELECT command denied to user 'testflush'@'localhost' for table 'user' diff --git a/tests/integrationtest/t/executor/simple.test b/tests/integrationtest/t/executor/simple.test index 992b9af49e..a34a87195d 100644 --- a/tests/integrationtest/t/executor/simple.test +++ b/tests/integrationtest/t/executor/simple.test @@ -165,6 +165,7 @@ disconnect conn1; # TestFlushPrivileges drop user if exists 'testflush'@'localhost'; CREATE USER 'testflush'@'localhost' IDENTIFIED BY ''; +SHOW GRANTS FOR 'testflush'@'localhost'; UPDATE mysql.User SET Select_priv='Y' WHERE User="testflush" and Host="localhost"; connect (conn1, localhost, testflush,,); --error 1142 @@ -529,4 +530,4 @@ drop user default_sm3_user; drop user default_sha2_user; drop user native_plugin_user; drop user default_sha2_role; -set global default_authentication_plugin = default; \ No newline at end of file +set global default_authentication_plugin = default;