chore: shadow PR for external contribution #37106 [DO NOT MERGE] #37222
Conversation
…github.com:AnnaHariprasad5123/appsmith into chore/external-contribution-37106
NilanshBansal
requested review from
nidhi-nair and
sondermanish
as code owners
WalkthroughThe pull request introduces enhancements to the Redis plugin by adding support for TLS (Transport Layer Security) configurations. A new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
NilanshBansal
changed the title
github-actions
bot
added
the
skip-changelog
github-actions
bot
added
Enhancement
github-actions
bot
added
Enhancement
NilanshBansal
changed the title
github-actions
bot
added
Enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (11)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/TlsConfiguration.java (2)
12-17: Add class-level documentationThis security-sensitive configuration class should have proper JavaDoc explaining its purpose and usage.
+/** + * Configuration class for TLS/SSL settings used in secure connections. + * This class encapsulates all TLS-related settings including certificate management + * and client authentication options. + */ @Getter @Setter
12-17: Consider making the configuration immutableFor security-sensitive configurations, consider making the class immutable by:
- Removing @Setter
- Using final fields
- Using a builder pattern for construction
@Getter -@Setter @ToString -@NoArgsConstructor -@AllArgsConstructor +@Builder +@RequiredArgsConstructor public class TlsConfiguration {app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceConfiguration.java (1)
37-39: Consider adding migration documentation.Since this adds a new field to a configuration class, it would be helpful to document any required database schema updates or migration steps.
app/server/appsmith-plugins/redisPlugin/src/main/resources/form.json (2)
97-129: Consider adding validation for certificate/key pairWhile the client authentication fields are properly configured with conditional visibility and encryption, consider adding validation to ensure both client certificate and key are provided when client authentication is enabled.
{ "label": "Upload Client Certificate", "configProperty": "datasourceConfiguration.tlsConfiguration.clientCertificateFile", "controlType": "FILE_PICKER", "encrypted": true, + "required": { + "path": "datasourceConfiguration.tlsConfiguration.requiresClientAuth", + "comparison": "EQUALS", + "value": true + }, "hidden": { "path": "datasourceConfiguration.tlsConfiguration.requiresClientAuth", "comparison": "EQUALS", "value": false } }
87-96: Consider adding file type validation for CA certificateFor better user experience and security, consider adding file type validation for the CA certificate upload.
{ "label": "Upload CA Certificate", "configProperty": "datasourceConfiguration.tlsConfiguration.caCertificateFile", "controlType": "FILE_PICKER", "encrypted": true, + "allowedFileTypes": [".pem", ".crt", ".cer"], "hidden": { "path": "datasourceConfiguration.tlsConfiguration.tlsEnabled", "comparison": "NOT_EQUALS", "value": true } }app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisTLSManager.java (4)
43-43: Specify TLS protocol version explicitlyConsider specifying the TLS protocol version explicitly by using
"TLSv1.2"or"TLSv1.3"instead of"TLS"to ensure you're using the desired version and to enhance security.
120-130: Use a secure library to handle private key parsingManually parsing the private key by stripping headers and decoding can be error-prone and might not handle all key formats. Consider using a robust library like Bouncy Castle's
PEMParserto load and parse PEM-formatted private keys securely and reliably.🧰 Tools
🪛 Gitleaks
122-123: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
40-42: Enhance exception handling for null TLS configuration flagsThrowing
IllegalArgumentExceptionwith a generic message may not provide enough context for debugging. Consider creating a custom exception or providing more detailed messages to help pinpoint configuration issues.
115-117: Ensure proper resource management for SSL resourcesWhile the
SSLContext,SSLSocketFactory, and related resources are managed by the JVM, it's good practice to verify that any created resources are appropriately released or closed if necessary to prevent potential memory leaks.app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.java (1)
266-275: Add unit tests for TLS handlingConsider adding unit tests for the new TLS configurations in both
datasourceCreateandvalidateDatasourcemethods to ensure all scenarios are properly tested.Also applies to: 313-339
app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java (1)
177-181: Refactor duplicated TLS configuration setup in test methodsThe TLS configuration setup code is duplicated across multiple test methods. Consider extracting it into a helper method to reduce code duplication and enhance maintainability.
Also applies to: 191-195, 205-209, 219-225
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceConfiguration.java(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/TlsConfiguration.java(1 hunks)app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.java(4 hunks)app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/exceptions/RedisErrorMessages.java(1 hunks)app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisTLSManager.java(1 hunks)app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisURIUtils.java(1 hunks)app/server/appsmith-plugins/redisPlugin/src/main/resources/form.json(1 hunks)app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java(9 hunks)
🧰 Additional context used
🪛 Gitleaks
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisTLSManager.java
122-123: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🔇 Additional comments (9)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceConfiguration.java (1)
37-39: LGTM! The TLS configuration field is well-placed and properly annotated.
The addition follows the established pattern in the class with appropriate JSON view annotations for serialization control.
Let's verify the TlsConfiguration class existence and structure:
✅ Verification successful
TLS configuration field is properly integrated and used correctly
The verification confirms:
TlsConfigurationclass exists in the same package with proper structure- Field is actively used in Redis plugin for TLS support
- Implementation follows consistent patterns with proper JSON view annotations
- Integration is complete with proper usage in
RedisPlugin,RedisURIUtils, andRedisTLSManager
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify TlsConfiguration class exists and has expected structure
ast-grep --pattern 'class TlsConfiguration {
$$$
}'
Length of output: 55
Script:
#!/bin/bash
# Let's try to find the TlsConfiguration class using ripgrep
rg "class TlsConfiguration" -A 5
# Also search for any references to TlsConfiguration to understand its location
rg "TlsConfiguration" --type java
Length of output: 5471
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/exceptions/RedisErrorMessages.java (2)
33-37: LGTM! Well-organized section header
The section header follows the established pattern in the file and clearly separates TLS-related error messages.
39-46: Error messages are clear and actionable
The error messages are well-structured and provide clear guidance to users about what's missing in their TLS configuration. Each message specifies both the problem and the required action.
Let's verify the consistency of these error messages with their usage:
✅ Verification successful
Error messages are properly integrated with validation logic
The error messages are correctly used in the Redis plugin's validation logic (RedisPlugin.java) to handle TLS configuration issues. The messages are also properly tested in RedisPluginTest.java to verify the validation behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of these error messages in validation logic
rg "CA_CERTIFICATE_MISSING_ERROR_MSG|TLS_CLIENT_AUTH_ENABLED_BUT_CLIENT_CERTIFICATE_MISSING_ERROR_MSG|TLS_CLIENT_AUTH_ENABLED_BUT_CLIENT_KEY_MISSING_ERROR_MSG" -A 2
Length of output: 4372
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisURIUtils.java (1)
16-17: LGTM! Correct Redis SSL scheme constant.
The REDIS_SSL_SCHEME constant correctly uses "rediss://" as per Redis protocol specifications.
app/server/appsmith-plugins/redisPlugin/src/main/resources/form.json (3)
61-65: LGTM: Section structure is well-defined
The TLS Configuration section is properly structured with a unique ID and follows the existing form pattern.
69-74: LGTM: TLS enable toggle is properly configured
The TLS enable checkbox is well-implemented with appropriate default value (false) and clear labeling.
75-96: Verify the conditional visibility logic for certificate fields
The certificate verification fields are properly encrypted and conditionally displayed. However, we should verify that the visibility conditions work as expected in the UI.
✅ Verification successful
Conditional visibility logic for TLS fields is correctly implemented
The form schema shows a well-structured and consistent implementation of conditional visibility:
- TLS certificate verification and CA certificate upload are correctly hidden when TLS is disabled
- Client certificate and key fields are properly hidden when client authentication is disabled
- All visibility conditions use appropriate comparison operators and paths
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that these visibility conditions are consistently implemented in the frontend
rg -A 2 "tlsConfiguration.tlsEnabled" app/client/src/
Length of output: 53
Script:
#!/bin/bash
# Let's search for the form configuration in the frontend
rg -g "*.{ts,tsx}" "tlsEnabled.*verifyTlsCertificate" app/client/src/
# Also search for any TLS configuration handling
rg -g "*.{ts,tsx}" "tlsConfiguration" app/client/src/
# Check if there are any form configurations related to Redis TLS
rg -g "*.{ts,tsx}" "Redis.*TLS" app/client/src/
# Look for any Redis plugin form handling
fd -e ts -e tsx -e json . | xargs rg "redisPlugin.*form"
Length of output: 234
Script:
#!/bin/bash
# Let's check the form schema structure in the plugin
cat app/server/appsmith-plugins/redisPlugin/src/main/resources/form.json
# Look for any form-related files in the Redis plugin
fd -e json . app/server/appsmith-plugins/redisPlugin/
# Check if there are any editor configurations
rg -g "*.{ts,tsx,json}" "datasourceConfiguration.*hidden"
Length of output: 4815
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.java (1)
313-339: TLS configuration validation looks good
The added validation logic for TLS configurations in the validateDatasource method appropriately checks for the necessary certificates and keys when TLS is enabled.
app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java (1)
174-229: New TLS validation tests are comprehensive
The added test cases effectively cover various TLS configuration scenarios, ensuring robust validation.
| @JsonView({Views.Public.class, FromRequest.class}) | ||
| Boolean tlsEnabled; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using primitive boolean with default values
The Boolean wrapper classes can be null, which might lead to NPEs. For configuration flags, it's better to use primitive boolean with default values.
- Boolean tlsEnabled;
+ boolean tlsEnabled = false;
- Boolean verifyTlsCertificate;
+ boolean verifyTlsCertificate = true;
- Boolean requiresClientAuth;
+ boolean requiresClientAuth = false;Also applies to: 22-23, 28-29
| @JsonView({Views.Public.class, FromRequest.class}) | ||
| UploadedFile caCertificateFile; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for certificate files
The certificate and key files should have validation constraints to ensure they're provided when required and have appropriate content types.
+ @javax.validation.constraints.NotNull(message = "CA certificate is required when TLS verification is enabled",
+ groups = TlsEnabledValidationGroup.class)
@JsonView({Views.Public.class, FromRequest.class})
UploadedFile caCertificateFile;
+ @javax.validation.constraints.NotNull(message = "Client certificate is required when client auth is enabled",
+ groups = ClientAuthEnabledValidationGroup.class)
@JsonView({Views.Public.class, FromRequest.class})
UploadedFile clientCertificateFile;
+ @javax.validation.constraints.NotNull(message = "Client key is required when client auth is enabled",
+ groups = ClientAuthEnabledValidationGroup.class)
@JsonView({Views.Public.class, FromRequest.class})
UploadedFile clientKeyFile;Also applies to: 31-35
|
|
||
| if (datasourceConfiguration.getTlsConfiguration() != null | ||
| && datasourceConfiguration.getTlsConfiguration().getTlsEnabled()) { | ||
| builder.append(REDIS_SSL_SCHEME); | ||
| } else { | ||
| builder.append(REDIS_SCHEME); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check for datasourceConfiguration parameter.
While the logic for TLS scheme selection is correct, consider adding a null check for the datasourceConfiguration parameter to prevent potential NullPointerException.
public static URI getURI(DatasourceConfiguration datasourceConfiguration) throws URISyntaxException {
+ if (datasourceConfiguration == null) {
+ throw new IllegalArgumentException("DatasourceConfiguration cannot be null");
+ }
StringBuilder builder = new StringBuilder();
if (datasourceConfiguration.getTlsConfiguration() != null📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (datasourceConfiguration.getTlsConfiguration() != null | |
| && datasourceConfiguration.getTlsConfiguration().getTlsEnabled()) { | |
| builder.append(REDIS_SSL_SCHEME); | |
| } else { | |
| builder.append(REDIS_SCHEME); | |
| } | |
| if (datasourceConfiguration == null) { | |
| throw new IllegalArgumentException("DatasourceConfiguration cannot be null"); | |
| } | |
| if (datasourceConfiguration.getTlsConfiguration() != null | |
| && datasourceConfiguration.getTlsConfiguration().getTlsEnabled()) { | |
| builder.append(REDIS_SSL_SCHEME); | |
| } else { | |
| builder.append(REDIS_SCHEME); | |
| } |
| trustManagers = new TrustManager[] { | ||
| new X509TrustManager() { | ||
| @Override | ||
| public X509Certificate[] getAcceptedIssuers() { | ||
| return new X509Certificate[0]; | ||
| } | ||
|
|
||
| @Override | ||
| public void checkClientTrusted(X509Certificate[] certs, String authType) {} | ||
|
|
||
| @Override | ||
| public void checkServerTrusted(X509Certificate[] certs, String authType) {} | ||
| } | ||
| }; |
There was a problem hiding this comment.
Review the implications of trusting all server certificates
When verifyTlsCertificate is set to false, the custom TrustManager trusts all server certificates without verification. This can pose significant security risks, such as man-in-the-middle attacks. Please ensure that this behavior is acceptable for your use case and consider adding warnings or documentation to inform users.
| if (datasourceConfiguration.getTlsConfiguration() != null | ||
| && !datasourceConfiguration | ||
| .getTlsConfiguration() | ||
| .getTlsEnabled()) { | ||
| JedisPool jedisPool = new JedisPool(poolConfig, uri, timeout); | ||
| log.debug(Thread.currentThread().getName() + ": Created Jedis pool."); | ||
| return jedisPool; | ||
| } else { | ||
| return createJedisPoolWithTLS(poolConfig, uri, timeout, datasourceConfiguration); | ||
| } |
There was a problem hiding this comment.
Review TLS configuration logic in datasourceCreate method
The condition in your if statement might not correctly handle cases where tlsConfiguration is null. Currently, if tlsConfiguration is null, the code defaults to creating a Jedis pool with TLS, which may not be intended. Consider revising the condition to ensure that TLS is enabled only when explicitly configured.
Apply this diff to correct the logic:
-if (datasourceConfiguration.getTlsConfiguration() != null
- && !datasourceConfiguration
- .getTlsConfiguration()
- .getTlsEnabled()) {
- JedisPool jedisPool = new JedisPool(poolConfig, uri, timeout);
- log.debug(Thread.currentThread().getName() + ": Created Jedis pool.");
- return jedisPool;
-} else {
+if (datasourceConfiguration.getTlsConfiguration() != null
+ && datasourceConfiguration
+ .getTlsConfiguration()
+ .getTlsEnabled()) {
+ return createJedisPoolWithTLS(poolConfig, uri, timeout, datasourceConfiguration);
+} else {
+ JedisPool jedisPool = new JedisPool(poolConfig, uri, timeout);
+ log.debug(Thread.currentThread().getName() + ": Created Jedis pool.");
+ return jedisPool;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (datasourceConfiguration.getTlsConfiguration() != null | |
| && !datasourceConfiguration | |
| .getTlsConfiguration() | |
| .getTlsEnabled()) { | |
| JedisPool jedisPool = new JedisPool(poolConfig, uri, timeout); | |
| log.debug(Thread.currentThread().getName() + ": Created Jedis pool."); | |
| return jedisPool; | |
| } else { | |
| return createJedisPoolWithTLS(poolConfig, uri, timeout, datasourceConfiguration); | |
| } | |
| if (datasourceConfiguration.getTlsConfiguration() != null | |
| && datasourceConfiguration | |
| .getTlsConfiguration() | |
| .getTlsEnabled()) { | |
| return createJedisPoolWithTLS(poolConfig, uri, timeout, datasourceConfiguration); | |
| } else { | |
| JedisPool jedisPool = new JedisPool(poolConfig, uri, timeout); | |
| log.debug(Thread.currentThread().getName() + ": Created Jedis pool."); | |
| return jedisPool; | |
| } |
| private TlsConfiguration addTLSConfiguration(DatasourceConfiguration datasourceConfiguration) { | ||
| TlsConfiguration tlsConfiguration = new TlsConfiguration(); | ||
| tlsConfiguration.setTlsEnabled(false); | ||
| return tlsConfiguration; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused parameter in addTLSConfiguration method
The parameter DatasourceConfiguration datasourceConfiguration in the addTLSConfiguration method is unused. Consider removing it to improve code clarity.
Apply this diff to remove the unused parameter:
-private TlsConfiguration addTLSConfiguration(DatasourceConfiguration datasourceConfiguration) {
+private TlsConfiguration addTLSConfiguration() {
TlsConfiguration tlsConfiguration = new TlsConfiguration();
tlsConfiguration.setTlsEnabled(false);
return tlsConfiguration;
}Update the method calls accordingly:
-datasourceConfiguration.setTlsConfiguration(addTLSConfiguration(datasourceConfiguration));
+datasourceConfiguration.setTlsConfiguration(addTLSConfiguration());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private TlsConfiguration addTLSConfiguration(DatasourceConfiguration datasourceConfiguration) { | |
| TlsConfiguration tlsConfiguration = new TlsConfiguration(); | |
| tlsConfiguration.setTlsEnabled(false); | |
| return tlsConfiguration; | |
| } | |
| private TlsConfiguration addTLSConfiguration() { | |
| TlsConfiguration tlsConfiguration = new TlsConfiguration(); | |
| tlsConfiguration.setTlsEnabled(false); | |
| return tlsConfiguration; | |
| } |
NilanshBansal
added
ok-to-test
Description
Fixes #26723
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11694971406
Commit: 951e3f1
Cypress dashboard.
Tags:
@tag.AllSpec:
Wed, 06 Nov 2024 02:20:17 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests