You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by GitBox <gi...@apache.org> on 2022/08/19 09:12:46 UTC
[GitHub] [incubator-kyuubi] pan3793 opened a new pull request, #3278: [KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code
pan3793 opened a new pull request, #3278:
URL: https://github.com/apache/incubator-kyuubi/pull/3278
<!--
Thanks for sending a pull request!
Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://kyuubi.readthedocs.io/en/latest/community/contributions.html
2. If the PR is related to an issue in https://github.com/apache/incubator-kyuubi/issues, add '[KYUUBI #XXXX]' in your PR title, e.g., '[KYUUBI #XXXX] Your PR title ...'.
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][KYUUBI #XXXX] Your PR title ...'.
-->
### _Why are the changes needed?_
<!--
Please clarify why the changes are needed. For instance,
1. If you add a feature, you can talk about the use case of it.
2. If you fix a bug, you can clarify why it is a bug.
-->
This is the followup of #3235, the main change is introdude `JdbcUtils` to simplify code, and allow empty password for Jdbc auth.
### _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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on pull request #3278: [KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code
Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #3278:
URL: https://github.com/apache/incubator-kyuubi/pull/3278#issuecomment-1221273651
Thanks, merging to master/1.6
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter commented on pull request #3278: [KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #3278:
URL: https://github.com/apache/incubator-kyuubi/pull/3278#issuecomment-1221241246
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/3278?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#3278](https://codecov.io/gh/apache/incubator-kyuubi/pull/3278?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e001b5b) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/97b14f8bc388cc9049fc7759facb04c790fa0a5a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (97b14f8) will **increase** coverage by `0.00%`.
> The diff coverage is `78.66%`.
```diff
@@ Coverage Diff @@
## master #3278 +/- ##
=========================================
Coverage 51.37% 51.38%
Complexity 13 13
=========================================
Files 468 469 +1
Lines 26222 26227 +5
Branches 3633 3629 -4
=========================================
+ Hits 13472 13476 +4
Misses 11467 11467
- Partials 1283 1284 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/3278?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...uthentication/JdbcAuthenticationProviderImpl.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2aWNlL2F1dGhlbnRpY2F0aW9uL0pkYmNBdXRoZW50aWNhdGlvblByb3ZpZGVySW1wbC5zY2FsYQ==) | `75.92% <72.09%> (-3.83%)` | :arrow_down: |
| [.../main/scala/org/apache/kyuubi/util/JdbcUtils.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS91dGlsL0pkYmNVdGlscy5zY2FsYQ==) | `87.50% <87.50%> (ø)` | |
| [...apache/kyuubi/engine/JpsApplicationOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvSnBzQXBwbGljYXRpb25PcGVyYXRpb24uc2NhbGE=) | `77.41% <0.00%> (-3.23%)` | :arrow_down: |
| [...pache/kyuubi/engine/YarnApplicationOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvWWFybkFwcGxpY2F0aW9uT3BlcmF0aW9uLnNjYWxh) | `64.81% <0.00%> (-1.86%)` | :arrow_down: |
| [...ache/kyuubi/operation/KyuubiOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uTWFuYWdlci5zY2FsYQ==) | `80.82% <0.00%> (-1.37%)` | :arrow_down: |
| [.../engine/spark/session/SparkSQLSessionManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9zZXNzaW9uL1NwYXJrU1FMU2Vzc2lvbk1hbmFnZXIuc2NhbGE=) | `78.48% <0.00%> (-1.27%)` | :arrow_down: |
| [...uthentication/LdapAuthenticationProviderImpl.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2aWNlL2F1dGhlbnRpY2F0aW9uL0xkYXBBdXRoZW50aWNhdGlvblByb3ZpZGVySW1wbC5zY2FsYQ==) | `95.23% <0.00%> (-0.15%)` | :arrow_down: |
| [...g/apache/kyuubi/operation/BatchJobSubmission.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vQmF0Y2hKb2JTdWJtaXNzaW9uLnNjYWxh) | `74.40% <0.00%> (ø)` | |
| [...a/org/apache/kyuubi/service/TFrontendService.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2aWNlL1RGcm9udGVuZFNlcnZpY2Uuc2NhbGE=) | `91.47% <0.00%> (+0.29%)` | :arrow_up: |
| [...yuubi/server/metadata/jdbc/JDBCMetadataStore.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvbWV0YWRhdGEvamRiYy9KREJDTWV0YWRhdGFTdG9yZS5zY2FsYQ==) | `89.96% <0.00%> (+0.69%)` | :arrow_up: |
| ... and [1 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/3278/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on pull request #3278: [KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code
Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #3278:
URL: https://github.com/apache/incubator-kyuubi/pull/3278#issuecomment-1221241439
@turboFei UT has been enhanced and CI passed now, please take a look again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on pull request #3278: [KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code
Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #3278:
URL: https://github.com/apache/incubator-kyuubi/pull/3278#issuecomment-1221263183
> Or I could fill anther PR for this very soon to fix this, better for release with 1.6.0.
You can send a followup PR if you have time :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3278: [KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3278:
URL: https://github.com/apache/incubator-kyuubi/pull/3278#discussion_r950703335
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImpl.scala:
##########
@@ -62,37 +73,27 @@ class JdbcAuthenticationProviderImpl(conf: KyuubiConf) extends PasswdAuthenticat
s" or contains blank space")
}
- if (StringUtils.isBlank(password)) {
- throw new AuthenticationException(s"Error validating, password is null" +
- s" or contains blank space")
- }
-
- var connection: Connection = null
- var queryStatement: PreparedStatement = null
-
try {
- connection = hikariDataSource.getConnection
-
- queryStatement = getAndPrepareQueryStatement(connection, user, password)
-
- val resultSet = queryStatement.executeQuery()
-
- if (resultSet == null || !resultSet.next()) {
- // auth failed
- throw new AuthenticationException(s"Password does not match or no such user. user:" +
- s" $user , password length: ${password.length}")
+ debug(s"prepared auth query: $preparedQuery")
+ JdbcUtils.executeQuery(preparedQuery) { stmt =>
+ stmt.setMaxRows(1) // minimum result size required for authentication
+ queryPlaceholders.zipWithIndex.foreach {
+ case (USERNAME_SQL_PLACEHOLDER, i) => stmt.setString(i + 1, user)
+ case (PASSWORD_SQL_PLACEHOLDER, i) => stmt.setString(i + 1, password)
+ case (p, _) => throw new IllegalArgumentException(
+ s"Unrecognized placeholder in Query SQL: $p")
+ }
+ } { resultSet =>
+ if (resultSet == null || !resultSet.next()) {
+ throw new AuthenticationException("Password does not match or no such user. " +
+ s"user: $user, password: $redactedPasswd")
Review Comment:
Good. And we have the fix and ut in https://github.com/apache/incubator-kyuubi/pull/3288 now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3278: [KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3278:
URL: https://github.com/apache/incubator-kyuubi/pull/3278#discussion_r950701752
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImpl.scala:
##########
@@ -62,37 +73,27 @@ class JdbcAuthenticationProviderImpl(conf: KyuubiConf) extends PasswdAuthenticat
s" or contains blank space")
}
- if (StringUtils.isBlank(password)) {
- throw new AuthenticationException(s"Error validating, password is null" +
- s" or contains blank space")
- }
-
- var connection: Connection = null
- var queryStatement: PreparedStatement = null
-
try {
- connection = hikariDataSource.getConnection
-
- queryStatement = getAndPrepareQueryStatement(connection, user, password)
-
- val resultSet = queryStatement.executeQuery()
-
- if (resultSet == null || !resultSet.next()) {
- // auth failed
- throw new AuthenticationException(s"Password does not match or no such user. user:" +
- s" $user , password length: ${password.length}")
+ debug(s"prepared auth query: $preparedQuery")
+ JdbcUtils.executeQuery(preparedQuery) { stmt =>
+ stmt.setMaxRows(1) // minimum result size required for authentication
+ queryPlaceholders.zipWithIndex.foreach {
+ case (USERNAME_SQL_PLACEHOLDER, i) => stmt.setString(i + 1, user)
+ case (PASSWORD_SQL_PLACEHOLDER, i) => stmt.setString(i + 1, password)
+ case (p, _) => throw new IllegalArgumentException(
+ s"Unrecognized placeholder in Query SQL: $p")
+ }
+ } { resultSet =>
+ if (resultSet == null || !resultSet.next()) {
+ throw new AuthenticationException("Password does not match or no such user. " +
+ s"user: $user, password: $redactedPasswd")
Review Comment:
@pan3793 It seems throwing woring redacted password with authdb password. It should throws with connection password. Let me fix it in next PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3278: [KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3278:
URL: https://github.com/apache/incubator-kyuubi/pull/3278#discussion_r950701886
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImpl.scala:
##########
@@ -62,37 +73,27 @@ class JdbcAuthenticationProviderImpl(conf: KyuubiConf) extends PasswdAuthenticat
s" or contains blank space")
}
- if (StringUtils.isBlank(password)) {
- throw new AuthenticationException(s"Error validating, password is null" +
- s" or contains blank space")
- }
-
- var connection: Connection = null
- var queryStatement: PreparedStatement = null
-
try {
- connection = hikariDataSource.getConnection
-
- queryStatement = getAndPrepareQueryStatement(connection, user, password)
-
- val resultSet = queryStatement.executeQuery()
-
- if (resultSet == null || !resultSet.next()) {
- // auth failed
- throw new AuthenticationException(s"Password does not match or no such user. user:" +
- s" $user , password length: ${password.length}")
+ debug(s"prepared auth query: $preparedQuery")
+ JdbcUtils.executeQuery(preparedQuery) { stmt =>
+ stmt.setMaxRows(1) // minimum result size required for authentication
+ queryPlaceholders.zipWithIndex.foreach {
+ case (USERNAME_SQL_PLACEHOLDER, i) => stmt.setString(i + 1, user)
+ case (PASSWORD_SQL_PLACEHOLDER, i) => stmt.setString(i + 1, password)
+ case (p, _) => throw new IllegalArgumentException(
+ s"Unrecognized placeholder in Query SQL: $p")
+ }
+ } { resultSet =>
+ if (resultSet == null || !resultSet.next()) {
+ throw new AuthenticationException("Password does not match or no such user. " +
+ s"user: $user, password: $redactedPasswd")
Review Comment:
Oh, my fault, thanks for catch this issue. Sure, let's fix it in followup
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] bowenliang123 commented on pull request #3278: [KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on PR #3278:
URL: https://github.com/apache/incubator-kyuubi/pull/3278#issuecomment-1221258315
@pan3793 May I suggest to change change a config name from `kyuubi.authentication.jdbc.username` to `kyuubi.authentication.jdbc.user` ?
`kyuubi.authentication.jdbc.username` was introduced in my initial code for issue 3222, but I think `user` is than suitable name than `username`, since`DriverManager.getConnection` takes`user` param in Java and we also have `kyuubi.metadata.store.jdbc.user` in KuyuubiConf.
Or I could fill anther PR for this very soon to fix this, better for release with 1.6.0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] turboFei commented on pull request #3278: [KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code
Posted by GitBox <gi...@apache.org>.
turboFei commented on PR #3278:
URL: https://github.com/apache/incubator-kyuubi/pull/3278#issuecomment-1220520052
```
JdbcAuthenticationProviderImplSuite:
- authenticate tests *** FAILED ***
"requirement failed: JDBC driver class is not configured." did not contain "JDBC url is not configured" (JdbcAuthenticationProviderImplSuite.scala:106)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3278: [KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3278:
URL: https://github.com/apache/incubator-kyuubi/pull/3278#discussion_r950671710
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImpl.scala:
##########
@@ -62,37 +77,27 @@ class JdbcAuthenticationProviderImpl(conf: KyuubiConf) extends PasswdAuthenticat
s" or contains blank space")
}
- if (StringUtils.isBlank(password)) {
- throw new AuthenticationException(s"Error validating, password is null" +
- s" or contains blank space")
- }
-
- var connection: Connection = null
- var queryStatement: PreparedStatement = null
-
try {
- connection = hikariDataSource.getConnection
-
- queryStatement = getAndPrepareQueryStatement(connection, user, password)
-
- val resultSet = queryStatement.executeQuery()
-
- if (resultSet == null || !resultSet.next()) {
- // auth failed
- throw new AuthenticationException(s"Password does not match or no such user. user:" +
- s" $user , password length: ${password.length}")
+ debug(s"prepared auth query: $preparedQuery")
+ JdbcUtils.executeQuery(preparedQuery) { stmt =>
Review Comment:
we can extend KyuubiConf to allow configuring arbitrary properties, but it's out of scope in this PR, better do it in new PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3278: [KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3278:
URL: https://github.com/apache/incubator-kyuubi/pull/3278#discussion_r950671375
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImpl.scala:
##########
@@ -101,104 +106,31 @@ class JdbcAuthenticationProviderImpl(conf: KyuubiConf) extends PasswdAuthenticat
debug(configLog("Driver Class", driverClass.orNull))
debug(configLog("JDBC URL", jdbcUrl.orNull))
- debug(configLog("Database username", jdbcUsername.orNull))
- debug(configLog("Database password length", jdbcUserPassword.getOrElse("").length.toString))
- debug(configLog("Query SQL", authQuerySql.orNull))
+ debug(configLog("Database username", username.orNull))
+ debug(configLog("Database password", redactedPasswd))
+ debug(configLog("Query SQL", authQuery.orNull))
// Check if JDBC parameters valid
- if (driverClass.isEmpty) {
- throw new IllegalArgumentException("JDBC driver class is not configured.")
- }
-
- if (jdbcUrl.isEmpty) {
- throw new IllegalArgumentException("JDBC url is not configured")
- }
-
- if (jdbcUsername.isEmpty || jdbcUserPassword.isEmpty) {
- throw new IllegalArgumentException("JDBC username or password is not configured")
+ require(driverClass.nonEmpty, "JDBC driver class is not configured.")
+ require(jdbcUrl.nonEmpty, "JDBC url is not configured.")
+ require(username.nonEmpty, "JDBC username is not configured")
+ // allow empty password
+ require(authQuery.nonEmpty, "Query SQL is not configured")
+
+ val query = authQuery.get.trim.toLowerCase
+ // allow simple select query sql only, complex query like CTE is not allowed
+ require(query.startsWith("select"), "Query SQL must start with 'SELECT'")
+ if (!query.contains("where")) {
+ warn("Query SQL does not contains 'WHERE' keyword")
}
-
- // Check Query SQL
- if (authQuerySql.isEmpty) {
- throw new IllegalArgumentException("Query SQL is not configured")
- }
- val querySqlInLowerCase = authQuerySql.get.trim.toLowerCase
- if (!querySqlInLowerCase.startsWith("select")) { // allow select query sql only
- throw new IllegalArgumentException("Query SQL must start with \"SELECT\"");
- }
- if (!querySqlInLowerCase.contains("where")) {
- warn("Query SQL does not contains \"WHERE\" keyword");
- }
- if (!querySqlInLowerCase.contains("${username}")) {
- warn("Query SQL does not contains \"${username}\" placeholder");
- }
- }
-
- private def getPlaceholderList(sql: String): List[String] = {
- SQL_PLACEHOLDER_REGEX.findAllMatchIn(sql)
- .map(m => m.matched)
- .toList
- }
-
- private def getAndPrepareQueryStatement(
- connection: Connection,
- user: String,
- password: String): PreparedStatement = {
-
- val preparedSql: String = {
- SQL_PLACEHOLDER_REGEX.replaceAllIn(authQuerySql.get, "?")
- }
- debug(s"prepared auth query sql: $preparedSql")
-
- val stmt = connection.prepareStatement(preparedSql)
- stmt.setMaxRows(1) // minimum result size required for authentication
-
- // Extract placeholder list and fill parameters to placeholders
- val placeholderList: List[String] = getPlaceholderList(authQuerySql.get)
- for (i <- placeholderList.indices) {
- val param = placeholderList(i) match {
- case USERNAME_SQL_PLACEHOLDER => user
- case PASSWORD_SQL_PLACEHOLDER => password
- case otherPlaceholder =>
- throw new IllegalArgumentException(
- s"Unrecognized Placeholder In Query SQL: $otherPlaceholder")
- }
-
- stmt.setString(i + 1, param)
- }
-
- stmt
- }
-
- private def closeDbConnection(connection: Connection, statement: Statement): Unit = {
- if (statement != null && !statement.isClosed) {
- try {
- statement.close()
- } catch {
- case e: Exception =>
- error("Cannot close PreparedStatement to auth database ", e)
- }
- }
-
- if (connection != null && !connection.isClosed) {
- try {
- connection.close()
- } catch {
- case e: Exception =>
- error("Cannot close connection to auth database ", e)
- }
+ if (!query.contains("${username}")) {
+ warn("Query SQL does not contains '${username}' placeholder")
}
Review Comment:
restored this warning message
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3278: [KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3278:
URL: https://github.com/apache/incubator-kyuubi/pull/3278#discussion_r950671539
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImpl.scala:
##########
@@ -62,37 +77,27 @@ class JdbcAuthenticationProviderImpl(conf: KyuubiConf) extends PasswdAuthenticat
s" or contains blank space")
}
- if (StringUtils.isBlank(password)) {
Review Comment:
I think we should allow empty password, and the auth query will fail the athentication is password is required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3278: [KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3278:
URL: https://github.com/apache/incubator-kyuubi/pull/3278#discussion_r950672292
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImpl.scala:
##########
@@ -62,37 +77,27 @@ class JdbcAuthenticationProviderImpl(conf: KyuubiConf) extends PasswdAuthenticat
s" or contains blank space")
}
- if (StringUtils.isBlank(password)) {
Review Comment:
agree
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3278: [KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3278:
URL: https://github.com/apache/incubator-kyuubi/pull/3278#discussion_r950701886
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImpl.scala:
##########
@@ -62,37 +73,27 @@ class JdbcAuthenticationProviderImpl(conf: KyuubiConf) extends PasswdAuthenticat
s" or contains blank space")
}
- if (StringUtils.isBlank(password)) {
- throw new AuthenticationException(s"Error validating, password is null" +
- s" or contains blank space")
- }
-
- var connection: Connection = null
- var queryStatement: PreparedStatement = null
-
try {
- connection = hikariDataSource.getConnection
-
- queryStatement = getAndPrepareQueryStatement(connection, user, password)
-
- val resultSet = queryStatement.executeQuery()
-
- if (resultSet == null || !resultSet.next()) {
- // auth failed
- throw new AuthenticationException(s"Password does not match or no such user. user:" +
- s" $user , password length: ${password.length}")
+ debug(s"prepared auth query: $preparedQuery")
+ JdbcUtils.executeQuery(preparedQuery) { stmt =>
+ stmt.setMaxRows(1) // minimum result size required for authentication
+ queryPlaceholders.zipWithIndex.foreach {
+ case (USERNAME_SQL_PLACEHOLDER, i) => stmt.setString(i + 1, user)
+ case (PASSWORD_SQL_PLACEHOLDER, i) => stmt.setString(i + 1, password)
+ case (p, _) => throw new IllegalArgumentException(
+ s"Unrecognized placeholder in Query SQL: $p")
+ }
+ } { resultSet =>
+ if (resultSet == null || !resultSet.next()) {
+ throw new AuthenticationException("Password does not match or no such user. " +
+ s"user: $user, password: $redactedPasswd")
Review Comment:
Oh, my fault, thanks for catching this issue. Sure, let's fix it in followup
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] bowenliang123 commented on pull request #3278: [KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on PR #3278:
URL: https://github.com/apache/incubator-kyuubi/pull/3278#issuecomment-1220478554
well learned from this pr. It is much resuable and elegant in scala style after refactoring.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3278: [KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3278:
URL: https://github.com/apache/incubator-kyuubi/pull/3278#discussion_r950058901
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImpl.scala:
##########
@@ -101,104 +106,31 @@ class JdbcAuthenticationProviderImpl(conf: KyuubiConf) extends PasswdAuthenticat
debug(configLog("Driver Class", driverClass.orNull))
debug(configLog("JDBC URL", jdbcUrl.orNull))
- debug(configLog("Database username", jdbcUsername.orNull))
- debug(configLog("Database password length", jdbcUserPassword.getOrElse("").length.toString))
- debug(configLog("Query SQL", authQuerySql.orNull))
+ debug(configLog("Database username", username.orNull))
+ debug(configLog("Database password", redactedPasswd))
+ debug(configLog("Query SQL", authQuery.orNull))
// Check if JDBC parameters valid
- if (driverClass.isEmpty) {
- throw new IllegalArgumentException("JDBC driver class is not configured.")
- }
-
- if (jdbcUrl.isEmpty) {
- throw new IllegalArgumentException("JDBC url is not configured")
- }
-
- if (jdbcUsername.isEmpty || jdbcUserPassword.isEmpty) {
- throw new IllegalArgumentException("JDBC username or password is not configured")
+ require(driverClass.nonEmpty, "JDBC driver class is not configured.")
+ require(jdbcUrl.nonEmpty, "JDBC url is not configured.")
+ require(username.nonEmpty, "JDBC username is not configured")
+ // allow empty password
+ require(authQuery.nonEmpty, "Query SQL is not configured")
+
+ val query = authQuery.get.trim.toLowerCase
+ // allow simple select query sql only, complex query like CTE is not allowed
+ require(query.startsWith("select"), "Query SQL must start with 'SELECT'")
+ if (!query.contains("where")) {
+ warn("Query SQL does not contains 'WHERE' keyword")
}
-
- // Check Query SQL
- if (authQuerySql.isEmpty) {
- throw new IllegalArgumentException("Query SQL is not configured")
- }
- val querySqlInLowerCase = authQuerySql.get.trim.toLowerCase
- if (!querySqlInLowerCase.startsWith("select")) { // allow select query sql only
- throw new IllegalArgumentException("Query SQL must start with \"SELECT\"");
- }
- if (!querySqlInLowerCase.contains("where")) {
- warn("Query SQL does not contains \"WHERE\" keyword");
- }
- if (!querySqlInLowerCase.contains("${username}")) {
- warn("Query SQL does not contains \"${username}\" placeholder");
- }
- }
-
- private def getPlaceholderList(sql: String): List[String] = {
- SQL_PLACEHOLDER_REGEX.findAllMatchIn(sql)
- .map(m => m.matched)
- .toList
- }
-
- private def getAndPrepareQueryStatement(
- connection: Connection,
- user: String,
- password: String): PreparedStatement = {
-
- val preparedSql: String = {
- SQL_PLACEHOLDER_REGEX.replaceAllIn(authQuerySql.get, "?")
- }
- debug(s"prepared auth query sql: $preparedSql")
-
- val stmt = connection.prepareStatement(preparedSql)
- stmt.setMaxRows(1) // minimum result size required for authentication
-
- // Extract placeholder list and fill parameters to placeholders
- val placeholderList: List[String] = getPlaceholderList(authQuerySql.get)
- for (i <- placeholderList.indices) {
- val param = placeholderList(i) match {
- case USERNAME_SQL_PLACEHOLDER => user
- case PASSWORD_SQL_PLACEHOLDER => password
- case otherPlaceholder =>
- throw new IllegalArgumentException(
- s"Unrecognized Placeholder In Query SQL: $otherPlaceholder")
- }
-
- stmt.setString(i + 1, param)
- }
-
- stmt
- }
-
- private def closeDbConnection(connection: Connection, statement: Statement): Unit = {
- if (statement != null && !statement.isClosed) {
- try {
- statement.close()
- } catch {
- case e: Exception =>
- error("Cannot close PreparedStatement to auth database ", e)
- }
- }
-
- if (connection != null && !connection.isClosed) {
- try {
- connection.close()
- } catch {
- case e: Exception =>
- error("Cannot close connection to auth database ", e)
- }
+ if (!query.contains("${username}")) {
+ warn("Query SQL does not contains '${username}' placeholder")
}
Review Comment:
```suggestion
}
if (!query.contains("${password}")) {
warn("Query SQL does not contains '${password}' placeholder")
}
```
How about adding check for ${password} placeholder here, as auth method checked password not blank in first place? @pan3793
##########
kyuubi-common/src/test/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImplSuite.scala:
##########
@@ -17,51 +17,55 @@
package org.apache.kyuubi.service.authentication
-import java.sql.{Connection, DriverManager}
+import java.sql.DriverManager
import java.util.Properties
import javax.security.sasl.AuthenticationException
+import javax.sql.DataSource
-import org.apache.kyuubi.KyuubiFunSuite
+import com.zaxxer.hikari.util.DriverDataSource
+
+import org.apache.kyuubi.{KyuubiFunSuite, Utils}
import org.apache.kyuubi.config.KyuubiConf
import org.apache.kyuubi.config.KyuubiConf._
+import org.apache.kyuubi.util.JdbcUtils
class JdbcAuthenticationProviderImplSuite extends KyuubiFunSuite {
protected val dbUser: String = "liangbowen"
protected val dbPasswd: String = "liangbowen"
- protected var jdbcUrl: String = _
+ protected val authDbName: String = "auth_db"
+ protected val jdbcUrl: String = s"jdbc:derby:memory:$authDbName;create=true"
Review Comment:
```suggestion
protected val dbUrl: String = s"jdbc:derby:memory:$authDbName"
protected val jdbcUrl: String = s"$dbUrl;create=true"
```
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImpl.scala:
##########
@@ -62,37 +77,27 @@ class JdbcAuthenticationProviderImpl(conf: KyuubiConf) extends PasswdAuthenticat
s" or contains blank space")
}
- if (StringUtils.isBlank(password)) {
- throw new AuthenticationException(s"Error validating, password is null" +
- s" or contains blank space")
- }
-
- var connection: Connection = null
- var queryStatement: PreparedStatement = null
-
try {
- connection = hikariDataSource.getConnection
-
- queryStatement = getAndPrepareQueryStatement(connection, user, password)
-
- val resultSet = queryStatement.executeQuery()
-
- if (resultSet == null || !resultSet.next()) {
- // auth failed
- throw new AuthenticationException(s"Password does not match or no such user. user:" +
- s" $user , password length: ${password.length}")
+ debug(s"prepared auth query: $preparedQuery")
+ JdbcUtils.executeQuery(preparedQuery) { stmt =>
Review Comment:
shall we a add a query time out for `JdbcUtils.executeQuery` to prevent blocking out of connection timeout ?
##########
kyuubi-common/src/test/scala/org/apache/kyuubi/util/JdbcUtilsSuite.scala:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.util
+
+import java.sql.DriverManager
+import java.util.Properties
+import javax.sql.DataSource
+
+import com.zaxxer.hikari.util.DriverDataSource
+
+import org.apache.kyuubi.KyuubiFunSuite
+
+class JdbcUtilsSuite extends KyuubiFunSuite {
+
+ private val connUrl = s"jdbc:derby:memory:jdbc_utils_test;create=true"
+ private val shutdownUrl = s"jdbc:derby:memory:jdbc_utils_test;shutdown=true"
Review Comment:
```suggestion
private val dbUrl = s"jdbc:derby:memory:jdbc_utils_test"
private val connUrl = s"$dbUrl;create=true"
private val shutdownUrl = s"$dbUrl;shutdown=true"
```
##########
kyuubi-common/src/test/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImplSuite.scala:
##########
@@ -70,16 +74,12 @@ class JdbcAuthenticationProviderImplSuite extends KyuubiFunSuite {
super.afterAll()
// cleanup db
- try {
+ Utils.tryLogNonFatalError {
DriverManager.getConnection(s"jdbc:derby:memory:$authDbName;shutdown=true")
Review Comment:
```suggestion
DriverManager.getConnection(s"$dbUrl;shutdown=true")
```
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImpl.scala:
##########
@@ -62,37 +77,27 @@ class JdbcAuthenticationProviderImpl(conf: KyuubiConf) extends PasswdAuthenticat
s" or contains blank space")
}
- if (StringUtils.isBlank(password)) {
Review Comment:
Shall we bring back password not blank checking? If auth method allow passing with missing password, the whole authentication relies on the confidentiality of username alone. @pan3793
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3278: [KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3278:
URL: https://github.com/apache/incubator-kyuubi/pull/3278#discussion_r950671375
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImpl.scala:
##########
@@ -101,104 +106,31 @@ class JdbcAuthenticationProviderImpl(conf: KyuubiConf) extends PasswdAuthenticat
debug(configLog("Driver Class", driverClass.orNull))
debug(configLog("JDBC URL", jdbcUrl.orNull))
- debug(configLog("Database username", jdbcUsername.orNull))
- debug(configLog("Database password length", jdbcUserPassword.getOrElse("").length.toString))
- debug(configLog("Query SQL", authQuerySql.orNull))
+ debug(configLog("Database username", username.orNull))
+ debug(configLog("Database password", redactedPasswd))
+ debug(configLog("Query SQL", authQuery.orNull))
// Check if JDBC parameters valid
- if (driverClass.isEmpty) {
- throw new IllegalArgumentException("JDBC driver class is not configured.")
- }
-
- if (jdbcUrl.isEmpty) {
- throw new IllegalArgumentException("JDBC url is not configured")
- }
-
- if (jdbcUsername.isEmpty || jdbcUserPassword.isEmpty) {
- throw new IllegalArgumentException("JDBC username or password is not configured")
+ require(driverClass.nonEmpty, "JDBC driver class is not configured.")
+ require(jdbcUrl.nonEmpty, "JDBC url is not configured.")
+ require(username.nonEmpty, "JDBC username is not configured")
+ // allow empty password
+ require(authQuery.nonEmpty, "Query SQL is not configured")
+
+ val query = authQuery.get.trim.toLowerCase
+ // allow simple select query sql only, complex query like CTE is not allowed
+ require(query.startsWith("select"), "Query SQL must start with 'SELECT'")
+ if (!query.contains("where")) {
+ warn("Query SQL does not contains 'WHERE' keyword")
}
-
- // Check Query SQL
- if (authQuerySql.isEmpty) {
- throw new IllegalArgumentException("Query SQL is not configured")
- }
- val querySqlInLowerCase = authQuerySql.get.trim.toLowerCase
- if (!querySqlInLowerCase.startsWith("select")) { // allow select query sql only
- throw new IllegalArgumentException("Query SQL must start with \"SELECT\"");
- }
- if (!querySqlInLowerCase.contains("where")) {
- warn("Query SQL does not contains \"WHERE\" keyword");
- }
- if (!querySqlInLowerCase.contains("${username}")) {
- warn("Query SQL does not contains \"${username}\" placeholder");
- }
- }
-
- private def getPlaceholderList(sql: String): List[String] = {
- SQL_PLACEHOLDER_REGEX.findAllMatchIn(sql)
- .map(m => m.matched)
- .toList
- }
-
- private def getAndPrepareQueryStatement(
- connection: Connection,
- user: String,
- password: String): PreparedStatement = {
-
- val preparedSql: String = {
- SQL_PLACEHOLDER_REGEX.replaceAllIn(authQuerySql.get, "?")
- }
- debug(s"prepared auth query sql: $preparedSql")
-
- val stmt = connection.prepareStatement(preparedSql)
- stmt.setMaxRows(1) // minimum result size required for authentication
-
- // Extract placeholder list and fill parameters to placeholders
- val placeholderList: List[String] = getPlaceholderList(authQuerySql.get)
- for (i <- placeholderList.indices) {
- val param = placeholderList(i) match {
- case USERNAME_SQL_PLACEHOLDER => user
- case PASSWORD_SQL_PLACEHOLDER => password
- case otherPlaceholder =>
- throw new IllegalArgumentException(
- s"Unrecognized Placeholder In Query SQL: $otherPlaceholder")
- }
-
- stmt.setString(i + 1, param)
- }
-
- stmt
- }
-
- private def closeDbConnection(connection: Connection, statement: Statement): Unit = {
- if (statement != null && !statement.isClosed) {
- try {
- statement.close()
- } catch {
- case e: Exception =>
- error("Cannot close PreparedStatement to auth database ", e)
- }
- }
-
- if (connection != null && !connection.isClosed) {
- try {
- connection.close()
- } catch {
- case e: Exception =>
- error("Cannot close connection to auth database ", e)
- }
+ if (!query.contains("${username}")) {
+ warn("Query SQL does not contains '${username}' placeholder")
}
Review Comment:
added warning message
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 closed pull request #3278: [KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code
Posted by GitBox <gi...@apache.org>.
pan3793 closed pull request #3278: [KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code
URL: https://github.com/apache/incubator-kyuubi/pull/3278
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on pull request #3278: [KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code
Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #3278:
URL: https://github.com/apache/incubator-kyuubi/pull/3278#issuecomment-1220464497
cc @bowenliang123
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org