Skip to content
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

#100 allow other password authenticator to work ZDM-530 #101

Merged
merged 2 commits into from
Jun 11, 2024

Conversation

weideng1
Copy link
Collaborator

@weideng1 weideng1 commented Feb 7, 2023

No description provided.

@joao-r-reis
Copy link
Collaborator

Did you check what each driver is doing? I checked C# and it ignores the name like the proxy would with this change. I assume python is the same and java driver 3.x too due to the tests that you ran but what about java driver 4.x?

@joao-r-reis
Copy link
Collaborator

@absurdfarce raised a valid point that we may not want to assume things about the authenticator so we could add an additional if here for the particular authenticator that we're trying to support here. Authenticators at the driver level are chosen by the user but this one in zdm proxy is pretty much internal because we don't really support any configuration on it.

In the end it may just be better to do like the drivers do and assume that if it's not the dse authenticator then the plaintext one should be used. @absurdfarce what's your opinion on this?

@joao-r-reis
Copy link
Collaborator

Also please create a GH issue and add this item to the changelog

@weideng1
Copy link
Collaborator Author

weideng1 commented Feb 7, 2023

Also please create a GH issue and add this item to the changelog

@joao-r-reis the GH issue was created as #100 before I opened this PR.

@absurdfarce
Copy link
Collaborator

Mentioned in Slack, mentioning here for sake of completeness. We've looked at the behaviour in the Python driver as well as 3.x and 4.x of the Java driver; all three follow the same basic pattern outlined by @weideng1 here.

@absurdfarce
Copy link
Collaborator

So the one major difference between the examples we've looked at so far is that (at least for the Python, Java 3.x and Java 4.x cases) each driver implements the authentication logic as a pluggable component. What I've described then is the default behaviour for the unmodified drivers but consumers are always free to define their own authn logic and implement a custom AuthProvider (or equivalent) to make it happen.

What we've been discussing on the zdm side isn't as pluggable. So there is always the potential for somebody to come along with a custom authenticator we can't support via the logic described above.

I don't see any obvious way around that besides supporting a pluggable authentication model in zdm as well. But that's a different question from whether we should make the change outlined here. This model changes us to "use the DSE authenticator if specified otherwise assume you have something that supports plaintext auth". As a default case that doesn't seem unreasonable to me.

@joao-r-reis
Copy link
Collaborator

Also please create a GH issue and add this item to the changelog

@joao-r-reis the GH issue was created as #100 before I opened this PR.

Add it to the changelog on this PR please

@joao-r-reis
Copy link
Collaborator

joao-r-reis commented Feb 8, 2023

And add Fix #100 to the PR description so it links and closes the GH issue automatically

@joao-r-reis
Copy link
Collaborator

And we should create a new milestone for an upcoming version that will have this change, if we are not sure what the version will be yet it can be created as TBD or 2.next or 2.x (and we will rename it when we decide which version it will be)

@jeromatron
Copy link
Collaborator

So the remaining item is just to add it to the changelog? After that, can we merge this and it can await a next release?

Copy link
Collaborator

@joao-r-reis joao-r-reis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When CI is done, feel free to merge

@lukasz-antoniak lukasz-antoniak merged commit 535490a into main Jun 11, 2024
8 checks passed
@lukasz-antoniak lukasz-antoniak deleted the more_auth branch June 11, 2024 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow other password authenticator to work
5 participants