[FIX] Match synced MySQL users by (username, host) to avoid collapsing multi-host accounts#1075
Open
erhanurgun wants to merge 1 commit intovitodeploy:3.xfrom
Open
Conversation
…g multi-host accounts SyncDatabaseUsers previously matched existing database_users rows by username only. MySQL and MariaDB treat 'user'@'localhost' and 'user'@'127.0.0.1' as independent accounts with potentially different passwords and grants, so collapsing them onto a single row caused silent data corruption: the databases column was overwritten in an order-dependent way, additional host variants were never inserted, and the stored host no longer matched the grants shown under it. The lookup now scopes by (username, host) whenever the handler returns a non-empty host, aligning sync with CreateDatabaseUser, UpdateDatabaseUser and DeleteDatabaseUser. PostgreSQL keeps the original username-only match because its get-users-list view returns an empty host (roles are host-agnostic); an unconditional composite filter would miss the existing UI-created row and insert a duplicate (rolname, '') record on every sync. Adds regression tests covering MySQL multi-host sync, MySQL sync idempotency and the PostgreSQL no-duplicate-row invariant.
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
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.
Summary
SyncDatabaseUsersso it scopes the existing-row lookup by(username, host)whenever the database handler returns a non-empty host, matching how MySQL/MariaDB identify accounts.'user'@'localhost'and'user'@'127.0.0.1'into a single record, dropping grants and misreporting hosts in the UI.get-users-listreturns''for host because roles are host-agnostic) is unaffected and does not start generating duplicate rows.app/Actions/Database/SyncDatabaseUsers.php; no schema or migration changes.Closes #1074
Why
MySQL treats
'user'@'localhost'and'user'@'127.0.0.1'as two independent accounts with potentially different passwords and grants, a very common setup once an app connects over TCP instead of the unix socket. The previouswhere('username', ...)->first()collapsed both onto a singledatabase_usersrow, so:databasesfield in an order-dependent way,nullbranch was unreachable after the first match),hostno longer corresponded to the grants shown under it in the UI.Adding
->where('host', $user[1])is the natural fix for MySQL/MariaDB, but PostgreSQL'sget-users-listview returns an empty string for host (roles are host-agnostic), whileCreateDatabaseUserstores PG users withhost = 'localhost'by default. An unconditional composite match would therefore miss the existing UI-created row on every sync and append a duplicate(rolname, '')row each time. Gating the host filter on$user[1] !== ''avoids that regression and keeps the existing behaviour for PostgreSQL exactly as before.The fix brings
SyncDatabaseUsersin line with the rest of the codebase (CreateDatabaseUser,UpdateDatabaseUser,DeleteDatabaseUser) which already scope by(username, host)for MySQL/MariaDB.Change
$user[1]is theHostcolumn already returned byDatabase::getUsers(); seeresources/views/ssh/services/database/{mysql,mariadb}/get-users-list.blade.php(returns actual host strings likelocalhost/127.0.0.1) andresources/views/ssh/services/database/postgresql/get-users-list.blade.php(selects'' AS host, triggering the new early-return branch).Test plan
vendor/bin/phpunit tests/Feature/DatabaseUserTest.php(adds three new regression tests covering MySQL multi-host sync, MySQL sync idempotency and the PostgreSQL no-duplicate-row invariant)'u'@'localhost'and'u'@'127.0.0.1'with differing grants, then hitPATCH /servers/{server}/database-users/sync.database_usersrow whosedatabasescolumn flips between the two grant sets depending on MySQL row order;hoststays stuck on the first one.(username, host)pair.'u'@'localhost'(the common case) continues to sync with no duplicate rows created.localhost), then hit the sync endpoint repeatedly. The existing row must keep being matched and updated; no(rolname, '')duplicates should be inserted.Notes for reviewers
Databasehandler implementation; that felt like over-engineering for a single call site.SyncDatabasesis not affected by the analogous class of bug because schema names are globally unique in MySQL/PostgreSQL, so itswhere('name', ...)lookup is already sufficient.(server_id, username, host)ondatabase_usersto make the invariant enforced at the DB layer, but that's a migration-level change and out of scope for this fix.