fix(plugin-mongodb): pin the auth database and create databases that persist - #1974
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
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.
Fixes #1970.
Creating a MongoDB database put the connection into an error state showing
[11] Authentication failed, with no way back. Creating the database was the trigger, not the bug. There are two independent defects.1. Browsing a MongoDB database re-authenticated against that database
MetadataConnectionPool.openEntryopens a separate driver per database. It copied the session's connection, setconnection.database = key.database, and connected. MongoDB uses the defaultsupportsConnectionPooling: true, so this ran on every sidebar refresh.That fabricated connection reached
MongoDBConnection.buildUri(), which setauthSourceto the target database whenever "Auth Database" was blank and the connection was not SRV. libmongoc sent the SCRAM handshake to a database the user does not exist in and returnedMONGOC_ERROR_CLIENT_AUTHENTICATE(11).Verified against a live MongoDB 7 server:
Creating a database auto-switches to it, which triggers the refresh, which opens the poisoned connection. Any database switch broke the same way on an authenticated server with a blank Auth Database, not just a freshly created one.
Fix. The pool no longer rewrites the connection's database for engines where that changes who you authenticate as. A pure
planConnectiondecides: connect with the configured database, thenswitchDatabaseto the target, the same shapeDatabaseManager+Health.restoreSchemaAndDatabasealready uses on every reconnect. Every other engine keeps today's behaviour byte for byte.This is gated by a new capability,
authenticationIsDatabaseScoped, defaulting tofalseand set only on MongoDB. It names the reason rather than the mechanism: the credentials authenticate against the database given at connect time. I deliberately did not reuserequiresReconnectForDatabaseSwitch, which is alsofalsefor MySQL, MariaDB, SQL Server, ClickHouse, Cassandra, Trino, Snowflake and others. "Can switch without reconnecting" is a different property from "the connect-time database does not change your identity", and flipping the path for eleven other engines buys nothing here.On the plugin side,
authSourcenow resolves throughMongoDBAuthSourceResolver(explicit value, then the configured database, thenadmin) and is computed once at construction, so a laterswitchDatabasecan never influence it.Because the app-side change restores what the plugin is handed, users get the fix from the app update alone, on whatever MongoDB plugin version they already have installed.
2. Create database deleted the database it created
createDatabaseinserted into__tablepro_initand then dropped it. MongoDB removes a database when its last collection goes. Verified live: present after the insert, gone after the drop. So "New Database" produced nothing even before the auth failure.Fix.
createDatabaseruns{"create": "<collection>"}and leaves the collection in place. An empty collection is enough to keep the database inlistDatabases, verified on 7.0.39, so no placeholder document is needed. The dialog now asks for a required First Collection, which is what MongoDB Compass does and for the same reason.MongoDBNameValidatorchecks MongoDB's documented name limits and the sheet's existing banner shows the message inline.3. The error message
A MongoDB auth failure now reads:
instead of
[11] Authentication failed, which is a raw libmongoc enum ordinal and the shape the HIG calls out by name ("Avoid writing a title that doesn't convey useful information, like 'Error 329347 occurred'"). The domain and code go to OSLog.PluginKit ABI: additive, no version bump
PluginCreateDatabaseFormSpec.FieldKindis@frozen, so a new case would be a breaking change forcing a bump and a re-release of every registry plugin. Instead the spec gains a nestedTextInputtype, atextInputsproperty, and a new init overload, with the existing init's signature untouched.scripts/check-pluginkit-abi.sh origin/mainreports additions only: the newTextInput, the new property, the new init. Nothing removed, no signature changed,FieldKindstill has exactly its two cases. Needs theabi-additivelabel.The plugin marks the field required while still defaulting the collection name to the database name when the value is absent, so an older app that does not render the field degrades quietly. The app and plugin releases need no ordering.
Tests
MongoDBAuthSourceResolverTests: the fallback chain, including that SRV always usesadmin.MongoDBNameValidatorTests: every forbidden character, byte-counted length limits, thesystem.prefix, the 255-byte namespace boundary.MongoDBCreateDatabasePlanTests: collection-name resolution, trimming, and both fallback paths, plus that the legacy form-spec init declares no text inputs.MetadataConnectionPoolPlanTests: the plan for a database-scoped engine, the redundant-switch case, a blank configured database, and that every other engine is unchanged.MetadataConnectionPoolTests: a driver that cannot switch databases is rejected rather than silently serving the wrong one.35 tests pass.
swiftlint lint --strictreports 0 violations in 1210 files. TheMongoDBDrivertarget builds.Release
The app-side changes ship with the next app release and fix the reported failure on their own. The plugin changes ship on the next
plugin-mongodb-v*tag. No other plugin needs re-releasing.Out of scope
NSError's failure-reason and recovery-suggestion tiers. Right, but a separate feature across every engine.requiresReconnectForDatabaseSwitch: falseand has database-scoped credentials, so it may have the same latent problem. Unverified, worth its own issue.Second commit: an unrelated flaky test this PR exposed
The first CI run failed on
SSHChannelRelay/ "Channel close terminates the relay", which this change does not touch. It is a pre-existing double-close in the test helper.SocketPair.close()closes both descriptors, andlocalHangupandtransportHangupeach callcloseB()first and thenclose()through theirdefer, sobis closed twice. The suite runs in parallel, so the second close releases a descriptor number the kernel may have already handed to a socket another test is polling. That test then sees a hangup it never caused, which is exactly the reported symptom:channelClosedexpected.channelClosedand got.localClosed.SocketPairnow tracks each descriptor and closes it once, and a failedsocketpair()no longer leaves the fds at 0, where closing them would have closed stdin.Reproduced locally at 1 failure in 5 runs before the fix, and 0 in 10 after. Kept as its own commit so it can be split out.