-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
chore: snowflake junit test cases added for key pair auth #34810
Conversation
WalkthroughThe changes in the Changes
Assessment against linked issues
Possibly related issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/server/appsmith-plugins/snowflakePlugin/src/test/java/com/external/plugins/SnowflakePluginTest.java (4 hunks)
Additional context used
Learnings (1)
app/server/appsmith-plugins/snowflakePlugin/src/test/java/com/external/plugins/SnowflakePluginTest.java (1)
Learnt from: AmanAgarwal041 PR: appsmithorg/appsmith#34491 File: app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java:320-338 Timestamp: 2024-06-26T11:16:09.077Z Learning: The `encryptedPassphrase` field in the `KeyPairAuth` class for Snowflake authentication is optional and does not require validation for emptiness.
Gitleaks
app/server/appsmith-plugins/snowflakePlugin/src/test/java/com/external/plugins/SnowflakePluginTest.java
68-70: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
Additional comments not posted (8)
app/server/appsmith-plugins/snowflakePlugin/src/test/java/com/external/plugins/SnowflakePluginTest.java (8)
53-75
: Potential Security Issue: Handling Private KeysThe method generates and returns a private key in PEM format. Ensure that this key is not logged or exposed in any way to avoid security risks.
Tools
Gitleaks
68-70: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
78-91
: Potential Security Issue: Handling Private KeysThe method generates and returns a private key in Base64 encoded format. Ensure that this key is not logged or exposed in any way to avoid security risks.
93-113
: LGTM!The method correctly sets up a
DatasourceConfiguration
with basic authentication details.
115-137
: LGTM!The method correctly sets up a
DatasourceConfiguration
with key pair authentication details.
351-366
: LGTM!The test method correctly validates the Hikari config setup for key pair authentication with a valid private key.
368-376
: LGTM!The test method correctly validates the error handling for key pair authentication with an invalid private key.
378-392
: LGTM!The test method correctly validates the Hikari config setup for basic authentication.
394-401
: LGTM!The test method correctly validates the reading of an encrypted private key.
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.
Just some minor changes. Rest LGTM!
...appsmith-plugins/snowflakePlugin/src/test/java/com/external/plugins/SnowflakePluginTest.java
Outdated
Show resolved
Hide resolved
...appsmith-plugins/snowflakePlugin/src/test/java/com/external/plugins/SnowflakePluginTest.java
Outdated
Show resolved
Hide resolved
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.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/server/appsmith-plugins/snowflakePlugin/src/test/java/com/external/plugins/SnowflakePluginTest.java (4 hunks)
Additional context used
Learnings (1)
app/server/appsmith-plugins/snowflakePlugin/src/test/java/com/external/plugins/SnowflakePluginTest.java (1)
Learnt from: AmanAgarwal041 PR: appsmithorg/appsmith#34491 File: app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java:320-338 Timestamp: 2024-06-26T11:16:09.077Z Learning: The `encryptedPassphrase` field in the `KeyPairAuth` class for Snowflake authentication is optional and does not require validation for emptiness.
Gitleaks
app/server/appsmith-plugins/snowflakePlugin/src/test/java/com/external/plugins/SnowflakePluginTest.java
80-82: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
Additional comments not posted (7)
app/server/appsmith-plugins/snowflakePlugin/src/test/java/com/external/plugins/SnowflakePluginTest.java (7)
16-16
: Imports approved.The imports for
SnowflakeKeyUtils
andSnowflakeBasicDataSource
are necessary and correctly used in the code.Also applies to: 24-24
90-110
: Method approved.The
createBasicAuthConfig
method correctly creates a datasource configuration for basic authentication.
112-134
: Method approved.The
createKeyPairAuthConfig
method correctly creates a datasource configuration for key pair authentication.
349-363
: Test case approved.The test case
testKeyAuthPairAuthValidPrivateKey_shouldCreateHikariConfigWithoutErrors
correctly validates the creation of Hikari configuration with a valid private key.
366-373
: Test case approved.The test case
testKeyAuthPairAuthInvalidPrivateKey_shouldThrowError
correctly validates that an error is thrown with an invalid private key.
376-389
: Test case approved.The test case
testBasicAuth_shouldCreateHikariConfigWithoutErrors
correctly validates the creation of Hikari configuration with basic authentication.
391-398
: Test case approved.The test case
testReadEncryptedPrivateKeyReturnsValidPrivateKey
correctly validates that reading an encrypted private key returns a validPrivateKey
instance.
...appsmith-plugins/snowflakePlugin/src/test/java/com/external/plugins/SnowflakePluginTest.java
Show resolved
Hide resolved
…g#34810) ## Description This PR adds JUnit test cases for snowflake Key auth pair feature added. Following cases have been added: - For basic authentication mechanism, when we do datasource create -> hikari config should have non empty username and password and datasource object inside hikari config should be null - For key pair authentication mechanism with valid private key -> hikari config should have datasource object which is of type SnowflakeBasicDatasource and username, password should be null. - For key pair authentication mechanism with invalid private key -> error should be thrown with appropriate error message - For key pair authentication mechanism with valid private key -> SnowflakeKeyUtils.readEncryptedPrivateKey should return valid instance of PrivateKey class. Note: Valid private key generated for above test cases is unencrypted private key (i.e. without passphrase). Adding test cases with private key having passphrase was a little complex than anticipated, hence will create a separate task and tackle it separately. Fixes appsmithorg#34692 _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.Perf" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9856542221> > Commit: 87074a7 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9856542221&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Perf` > Spec: > <hr>Tue, 09 Jul 2024 12:20:08 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Tests** - Implemented new tests for key pair authentication with both valid and invalid private keys. - Added tests for basic authentication. - Introduced tests for reading an encrypted private key. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: “sneha122” <“[email protected]”>
Description
This PR adds JUnit test cases for snowflake Key auth pair feature added. Following cases have been added:
Note: Valid private key generated for above test cases is unencrypted private key (i.e. without passphrase). Adding test cases with private key having passphrase was a little complex than anticipated, hence will create a separate task and tackle it separately.
Fixes #34692
or
Fixes
Issue URL
Warning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Perf"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9856542221
Commit: 87074a7
Cypress dashboard.
Tags:
@tag.Perf
Spec:
Tue, 09 Jul 2024 12:20:08 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit