-
Notifications
You must be signed in to change notification settings - Fork 373
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes inability to authenticate through the older cmdQuery mechanism #4066
Open
Evengard
wants to merge
1
commit into
FerretDB:main
Choose a base branch
from
Evengard:fix-cmdQuery-auth
base: main
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's return the error for the case where
err != nil
. 🤗Let's also handle the case where
mechanism != "PLAIN"
. I think we can handle it similarly to how it's done inmsg_saslstart.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to provide extreme backwards compatibility by just falling back to the "stub" behavior if something went wrong, as I have no ideas if that would break any other clients. But I don't mind returning explicit errors here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your quick response 🤗. Let us check, @b1ron do you know which other driver uses
cmd_query.go
? My guess is, it doesn't work for those drivers? If that's the case having an appropriate error is sensible here for a client attempting to authenticate with other mechanisms.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends on the specific driver and version. Looking at this compatibility matrix drivers that do not support MongoDB 6.0 will use legacy opcodes. I suspect @Evengard is using mongocsharpdriver version 2.15 or older.
Fixing #3121 might be easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Narrator: It wasn't actually removed.
But yeah, to move forward, we have to know the driver version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to force an OP_MSG using version 2.19.1, strange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is I can force an OP_MSG authentication with StableAPI, it's the inserts which bugs then.
I can do a PR in attempts to ignore theese apiVersions parameters probably instead. But looking that the actual issue seemed abandoned wasn't sure it is the way to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#4067 Went ahead with the StableAPI parameter ignoring... It works with 2.23.1 too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Evengard, and apologies for my confusion. I was able to reproduce the error in both 2.19.1 and 2.23.1.
#4067 fixes the issue I also tested it. The core team will review it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should do FerretDB/dance#769 first