server: restore session on change user auth failure (#69692) - #70229
server: restore session on change user auth failure (#69692)#70229ti-chi-bot wants to merge 1 commit into
Conversation
Signed-off-by: bb7133 <bb7133@gmail.com>
ti-chi-bot
added
release-note
|
[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 |
📝 WalkthroughWalkthrough
ChangesAtomic change-user authentication
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.12.2)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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/server/conn_test.go`:
- Around line 1754-1766: The existing handleChangeUser test only covers an
unknown user and does not exercise rollback after authentication failure. Update
the test around cc.handleChangeUser to create or use a valid password-protected
user, provide an invalid authentication response, and then assert the existing
old-session invariants for tc, cc.user, cc.dbname, and session username.
🪄 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 Plus
Run ID: 9d5cdd89-1f6c-490d-94cf-d74dfdc0b574
📒 Files selected for processing (2)
pkg/server/conn.gopkg/server/conn_test.go
| data := []byte{} | ||
| data = append(data, "missing_user"...) | ||
| data = append(data, 0) | ||
| data = append(data, 0) | ||
| data = append(data, "new_db"...) | ||
| data = append(data, 0) | ||
| data = append(data, 0, 0) | ||
| err = cc.handleChangeUser(context.Background(), data) | ||
| require.Error(t, err) | ||
| require.Same(t, tc, cc.getCtx()) | ||
| require.Equal(t, "root", cc.user) | ||
| require.Equal(t, "old_db", cc.dbname) | ||
| require.Equal(t, "root", cc.ctx.GetSessionVars().User.Username) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover rollback after credential authentication fails.
missing_user fails in checkAuthPlugin before openSessionAndDoAuth, leaving the rollback at pkg/server/conn.go Line 2659 untested. Add a valid password-protected user and an invalid auth response, then assert the same old-session invariants.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/server/conn_test.go` around lines 1754 - 1766, The existing
handleChangeUser test only covers an unknown user and does not exercise rollback
after authentication failure. Update the test around cc.handleChangeUser to
create or use a valid password-protected user, provide an invalid authentication
response, and then assert the existing old-session invariants for tc, cc.user,
cc.dbname, and session username.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #70229 +/- ##
================================================
Coverage ? 53.6091%
================================================
Files ? 1848
Lines ? 666302
Branches ? 0
================================================
Hits ? 357199
Misses ? 283583
Partials ? 25520
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@ti-chi-bot: The following test 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. |
ti-chi-bot
Bot
added
cherry-pick-approved
3 participants
This is an automated cherry-pick of #69692
What problem does this PR solve?
Issue Number: close #69691
Problem Summary:
COM_CHANGE_USERreplaces the connection user/database and opens a new session before the new identity has fully passed authentication. If auth-plugin validation or authentication fails, the function returns the error while the new session can remain attached to the connection, leaving the connection in an inconsistent state.What changed and how does it work?
This PR makes the change-user session switch failure-safe:
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
Bug Fixes
Tests