[KYUUBI #3222] JDBC Authentication Provider for server #3235
Conversation
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
|
Can we add a derby/sqlite based ut? |
The initial idea of PR is intended to provide an basic implementation for JDBC auth out of box without depending on other packages other than driver and db itself. |
|
"ut" is abbr of "unit testing", we need to add unit testing to verify the new feature works as expected and avoid breaking accidentally by future code changes. |
|
yes, will add the test suite for it referring the LdapAuthenticationProviderImplSuite and other test cases with derby. |
|
unit tests on derby added. |
Codecov Report
@@ Coverage Diff @@
## master #3235 +/- ##
============================================
+ Coverage 50.94% 51.04% +0.10%
Complexity 13 13
============================================
Files 469 470 +1
Lines 26123 26224 +101
Branches 3611 3626 +15
============================================
+ Hits 13309 13387 +78
- Misses 11546 11554 +8
- Partials 1268 1283 +15
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Hi @bowenliang123, you can use |
|
@pan3793 Thanks for the reminding. And struggling with the differences with IDEA default code format actions and project style checks. I will fix them very soon. The main part of code, test and docs should be okay. |
pan3793
reviewed
Aug 16, 2022
...est/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImplSuite.scala
Outdated
Show resolved
Hide resolved
pan3793
reviewed
Aug 16, 2022
...est/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImplSuite.scala
Show resolved
Hide resolved
pan3793
reviewed
Aug 16, 2022
...src/main/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImpl.scala
Outdated
Show resolved
Hide resolved
pan3793
reviewed
Aug 16, 2022
...src/main/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImpl.scala
Show resolved
Hide resolved
pan3793
reviewed
Aug 16, 2022
...src/main/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImpl.scala
Outdated
Show resolved
Hide resolved
pan3793
reviewed
Aug 16, 2022
pan3793
approved these changes
Aug 17, 2022
|
Thanks, merging to master |
|
Thanks for contribution! |
3 tasks
pan3793
added a commit
that referenced
this pull request
Aug 20, 2022
### _Why are the changes needed?_ This is the followup of #3235, the main change is introdude `JdbcUtils` to simplify code, and allow empty password for Jdbc auth. Jdbc connection pool has been removed because `JdbcAuthenticationProviderImpl` will be created on each connection, we can improve to use singleton in the future ### _How was this patch tested?_ - [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible - [ ] Add screenshots for manual tests if appropriate - [x] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request Closes #3278 from pan3793/jdbc-followup. Closes #3222 2863cae [Cheng Pan] Update kyuubi-common/src/test/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImplSuite.scala 51a9c45 [Cheng Pan] Update kyuubi-common/src/test/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImplSuite.scala eee3c55 [Cheng Pan] Update kyuubi-common/src/test/scala/org/apache/kyuubi/util/JdbcUtilsSuite.scala d02bb99 [Cheng Pan] nit e001b5b [Cheng Pan] nit 8cf5cd6 [Cheng Pan] nit 032f2df [Cheng Pan] nit 8a42f18 [Cheng Pan] nit c7893fd [Cheng Pan] JdbcUtilsSuite f97f2d9 [Cheng Pan] remove pool a8812d0 [Cheng Pan] move render result set to test 83d7d4c [Cheng Pan] fix ut db787a4 [Cheng Pan] nit 864f9dd [Cheng Pan] nit b60decf [Cheng Pan] nit 8c66e0b [Cheng Pan] nit 2063c43 [Cheng Pan] [KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code Authored-by: Cheng Pan <[email protected]> Signed-off-by: Cheng Pan <[email protected]>
pan3793
added a commit
that referenced
this pull request
Aug 20, 2022
### _Why are the changes needed?_ This is the followup of #3235, the main change is introdude `JdbcUtils` to simplify code, and allow empty password for Jdbc auth. Jdbc connection pool has been removed because `JdbcAuthenticationProviderImpl` will be created on each connection, we can improve to use singleton in the future ### _How was this patch tested?_ - [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible - [ ] Add screenshots for manual tests if appropriate - [x] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request Closes #3278 from pan3793/jdbc-followup. Closes #3222 2863cae [Cheng Pan] Update kyuubi-common/src/test/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImplSuite.scala 51a9c45 [Cheng Pan] Update kyuubi-common/src/test/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImplSuite.scala eee3c55 [Cheng Pan] Update kyuubi-common/src/test/scala/org/apache/kyuubi/util/JdbcUtilsSuite.scala d02bb99 [Cheng Pan] nit e001b5b [Cheng Pan] nit 8cf5cd6 [Cheng Pan] nit 032f2df [Cheng Pan] nit 8a42f18 [Cheng Pan] nit c7893fd [Cheng Pan] JdbcUtilsSuite f97f2d9 [Cheng Pan] remove pool a8812d0 [Cheng Pan] move render result set to test 83d7d4c [Cheng Pan] fix ut db787a4 [Cheng Pan] nit 864f9dd [Cheng Pan] nit b60decf [Cheng Pan] nit 8c66e0b [Cheng Pan] nit 2063c43 [Cheng Pan] [KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code Authored-by: Cheng Pan <[email protected]> Signed-off-by: Cheng Pan <[email protected]> (cherry picked from commit d0f75e8) Signed-off-by: Cheng Pan <[email protected]>
pan3793
pushed a commit
that referenced
this pull request
Aug 22, 2022
…C Authentication Provider ### _Why are the changes needed?_ To fix the config name and placeholder with `username` introduced in #3235 violate this convention as in JDBC driver use `user` keyword used for connection user rather than `username`, 1. change config name from `kyuubi.authentication.jdbc.username` to `kyuubi.authentication.jdbc.user` 2. change placeholder from `${username}` to `${user}` 3. update docs and config description related to above changes, and sync the update in jdbc auth docs statement details to config docs. 4. fix error in throwing AuthenticationException with auth db password. ut added for the fix. 5. other minor update in docs of custom auth ### _How was this patch tested?_ - [x] Add some test cases that check the changes thoroughly including negative and positive cases if possible - [ ] Add screenshots for manual tests if appropriate - [x] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request Closes #3288 from bowenliang123/jdbc-auth-config-update. Closes #3222 502703f [Bowen Liang] skip map for placeholder value lookup 3733be4 [liangbowen] nit ab00525 [liangbowen] nit 2301c4f [liangbowen] fix ut of jdbc auth with wrong_password 06f0c1b [liangbowen] remove redundant docs ec4565b [liangbowen] remove redundant docs ae1cce2 [liangbowen] fix compilation error of configLog 5d14103 [liangbowen] simplify configLog 6678e65 [liangbowen] reformat 52c1038 [liangbowen] simplify placeholder checking 21c2d5e [liangbowen] check whether placeholders in supported list before conn establishment or authenticate 7db0adf [liangbowen] ut for unknown placeholder 657de6a [liangbowen] nit 736b3f2 [liangbowen] refactoring placeholder value lookup, for preventing setString multiple times with "i+1" 86c8912 [liangbowen] setMaxRows after prepare placeholder, to postpone operation on jdbc conn 115fae5 [liangbowen] increase test code coverage b45b28c [liangbowen] resultSet returned by executeQuery is never null e1c0727 [liangbowen] update ut for redactPassword in JdbcUtils b4a52e2 [liangbowen] fix typo in docs of custom auth 371c2c6 [liangbowen] move redactPassword method to JdbcUtils and add ut. a4973c5 [liangbowen] reformat code 486e150 [liangbowen] fix error in throwing AuthenticationException with auth db password. add ut for the fix. efced90 [liangbowen] update settings.md ef97e35 [liangbowen] add SELECT prefix hint for doc of kyuubi.authentication.jdbc.query 025f94c [liangbowen] fix username to user in JdbcAuthenticationProviderImpl by 1. use config name `kyuubi.authentication.jdbc.user`, 2. use ${user} placeholder instead of ${username} Lead-authored-by: liangbowen <[email protected]> Co-authored-by: Bowen Liang <[email protected]> Signed-off-by: Cheng Pan <[email protected]>
pan3793
pushed a commit
that referenced
this pull request
Aug 22, 2022
…C Authentication Provider ### _Why are the changes needed?_ To fix the config name and placeholder with `username` introduced in #3235 violate this convention as in JDBC driver use `user` keyword used for connection user rather than `username`, 1. change config name from `kyuubi.authentication.jdbc.username` to `kyuubi.authentication.jdbc.user` 2. change placeholder from `${username}` to `${user}` 3. update docs and config description related to above changes, and sync the update in jdbc auth docs statement details to config docs. 4. fix error in throwing AuthenticationException with auth db password. ut added for the fix. 5. other minor update in docs of custom auth ### _How was this patch tested?_ - [x] Add some test cases that check the changes thoroughly including negative and positive cases if possible - [ ] Add screenshots for manual tests if appropriate - [x] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request Closes #3288 from bowenliang123/jdbc-auth-config-update. Closes #3222 502703f [Bowen Liang] skip map for placeholder value lookup 3733be4 [liangbowen] nit ab00525 [liangbowen] nit 2301c4f [liangbowen] fix ut of jdbc auth with wrong_password 06f0c1b [liangbowen] remove redundant docs ec4565b [liangbowen] remove redundant docs ae1cce2 [liangbowen] fix compilation error of configLog 5d14103 [liangbowen] simplify configLog 6678e65 [liangbowen] reformat 52c1038 [liangbowen] simplify placeholder checking 21c2d5e [liangbowen] check whether placeholders in supported list before conn establishment or authenticate 7db0adf [liangbowen] ut for unknown placeholder 657de6a [liangbowen] nit 736b3f2 [liangbowen] refactoring placeholder value lookup, for preventing setString multiple times with "i+1" 86c8912 [liangbowen] setMaxRows after prepare placeholder, to postpone operation on jdbc conn 115fae5 [liangbowen] increase test code coverage b45b28c [liangbowen] resultSet returned by executeQuery is never null e1c0727 [liangbowen] update ut for redactPassword in JdbcUtils b4a52e2 [liangbowen] fix typo in docs of custom auth 371c2c6 [liangbowen] move redactPassword method to JdbcUtils and add ut. a4973c5 [liangbowen] reformat code 486e150 [liangbowen] fix error in throwing AuthenticationException with auth db password. add ut for the fix. efced90 [liangbowen] update settings.md ef97e35 [liangbowen] add SELECT prefix hint for doc of kyuubi.authentication.jdbc.query 025f94c [liangbowen] fix username to user in JdbcAuthenticationProviderImpl by 1. use config name `kyuubi.authentication.jdbc.user`, 2. use ${user} placeholder instead of ${username} Lead-authored-by: liangbowen <[email protected]> Co-authored-by: Bowen Liang <[email protected]> Signed-off-by: Cheng Pan <[email protected]>
|
Glad to have this feature metioned as one of highlights in v1.6.0-incubating release notes. 🎉 |
|
👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Why are the changes needed?
Add JDBC authentication provider as implementation of PasswdAuthenticationProvider out of box.
Compared to currently support authentication methods like Kerberos and LDAP, JDBC source is a much easy and stable source for practical deployment.
The solution should provide:
easy to use and config jdbc connection details
handy to customize query statements for authentication
Adding JDBC to AuthMethods enum and JDBCPasswdAuthenticationProvider into package org.apache.kyuubi.service.authentication with listed features:
SELECT 1 from user_pass_hash where username = ${user} and password = MD5(${password})How was this patch tested?
Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before make a pull request