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
Allow MongoDB datasources without username/password #3149
Conversation
} | ||
|
||
} else { | ||
if (authType == null || !VALID_AUTH_TYPES.contains(authType)) { |
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.
Should we be adding a default authentication type of "None" for this change?
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.
Yeah I debated doing that, but figured adding another auth type would just make the fact that we support password-less auth less discoverable. It's just more natural to have the username and password input fields visible, and yet set to empty string. Case in point, it was how a user tried to do it and when that failed, reached out on Discord :)
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 can solve for discovery by keeping it as the default value. It's so that the UX is similar across plugins. For users that want to use authentication they will have to switch to another mechanism, and then we can perform conditional validation. Without the flag, the backend won't have a way to differentiate between no auth (which can belong to any mechanism) and incorrect configurations.
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'm not too sure I agree. Firstly, more than consistency, I'd argue we should make the most used flow be the default. Which in this case, is to use a password (instead of no password). Secondly, I don't see much value in the backend being able to differentiate. Regarding being able to identify invalid configurations, I'd argue that an invalid configuration here happens when the user has configured two things that are not in sync. Instead we could just let them keep the password empty, and we do the right thing. Otherwise we'll end up adding a step like "You have empty password, please fill it up" and then the user now needs to discover the no password option. If we make it the default though, the user might be searching for the password input and hopefully will figure out that they have to change the auth method for that to show up.
To me, this feels the simple solution. Both implementation wise and UX wise. (Unless I misunderstood something). What say?
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.
One of those moments when I'm not too strongly inclined towards either solutions because my sense of UX is from a "pain is gain" dimension. We should probably take inputs from the rest for this.
But two things I wanted to clarify here:
- In the REST API we explicitly ask users whether they want to be using authentication. After which they get to use a default type. In the mongodb experience, they're shown default authentication with a specific type attached. There is no indication to let them know that leaving the fields empty will disregard the chosen authentication mechanism.
- From the backend's perspective, this is probably more of a cleanliness thing for the data. The only way to identify a no-authentication mongodb datasource will now be to do a check on both username and password instead of type. OTOH checks on specific mechanisms will also need to check for username and password validation (in case of any kind of migrations in the future)
I'm going to go ahead an approve the PR anyway, so you can merge this in unless there's any further discussion that needs to happen @mohanarpit
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'm going to merge this now. Perhaps based off of user feedback or otherwise, we'll know better and can then reiterate. Thanks.
No description provided.