fix: SMTP datasource should work without username and password#37319
Conversation
WalkthroughThe changes in the Changes
Assessment against linked issues
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
app/server/appsmith-plugins/smtpPlugin/src/main/java/com/external/plugins/SmtpPlugin.java (2)
Line range hint
261-267: Remove mandatory authentication validation.The validateDatasource() method still enforces mandatory authentication, which contradicts the PR objective of allowing SMTP without credentials.
Apply this diff to fix the validation:
DBAuth authentication = (DBAuth) datasourceConfiguration.getAuthentication(); -if (authentication == null - || !StringUtils.hasText(authentication.getUsername()) - || !StringUtils.hasText(authentication.getPassword())) { - invalids.add(new AppsmithPluginException(AppsmithPluginError.PLUGIN_AUTHENTICATION_ERROR).getMessage()); -}
Line range hint
276-291: Improve error message for authentication failures.While the conditional error handling is correct, consider making the error message more specific when authentication fails.
Consider updating SMTPErrorMessages.DS_AUTHENTICATION_FAILED_ERROR_MSG to be more descriptive:
-"Invalid authentication credentials. Please check datasource configuration." +"Authentication failed. Please verify the username and password in the datasource configuration, or disable authentication if not required."app/server/appsmith-plugins/smtpPlugin/src/test/java/com/external/plugins/SmtpPluginTest.java (1)
134-137: LGTM! Consider adding an integration test.The variable rename and assertion changes correctly align with the PR objective of allowing SMTP without authentication. However, consider adding an integration test that verifies the actual SMTP connection succeeds without credentials.
@Test public void testExecute_withoutAuthentication_shouldSucceed() { DatasourceConfiguration noAuthConfig = createDatasourceConfiguration(); noAuthConfig.setAuthentication(null); ActionConfiguration actionConfiguration = createActionConfiguration(); Mono<ActionExecutionResult> resultMono = pluginExecutor .datasourceCreate(noAuthConfig) .flatMap(session -> pluginExecutor.execute(session, noAuthConfig, actionConfiguration)); StepVerifier.create(resultMono) .assertNext(result -> assertTrue(result.getIsExecutionSuccess())) .verifyComplete(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/server/appsmith-plugins/smtpPlugin/src/main/java/com/external/plugins/SmtpPlugin.java(3 hunks)app/server/appsmith-plugins/smtpPlugin/src/test/java/com/external/plugins/SmtpPluginTest.java(1 hunks)
🔇 Additional comments (1)
app/server/appsmith-plugins/smtpPlugin/src/main/java/com/external/plugins/SmtpPlugin.java (1)
215-232: LGTM! Authentication handling looks good.
The conditional authentication implementation correctly handles both authenticated and unauthenticated scenarios.
| return Mono.fromCallable(() -> { | ||
| Set<String> invalids = new HashSet<>(); | ||
|
|
||
| boolean isAuthRequired = connection.getProperty("mail.smtp.auth") != null |
There was a problem hiding this comment.
@shanid544 validateDatasource method has not been updated, it will still throw error when username and password is empty, please check that once
There was a problem hiding this comment.
@sneha122 Sorry, I reverted by mistake, for testing some scenarios, will update
| invalids.add(SMTPErrorMessages.DS_NO_SUCH_PROVIDER_ERROR_MSG); | ||
| } catch (AuthenticationFailedException e) { | ||
| invalids.add(SMTPErrorMessages.DS_AUTHENTICATION_FAILED_ERROR_MSG); | ||
| if (isAuthRequired) { |
There was a problem hiding this comment.
@shanid544 Can you please explain why this isAuthRequired check is required here? In case of no username and password this exception would never occur right? So why this specific condition?
There was a problem hiding this comment.
@sneha122 This is correct. if we make auth false, no need of this flag, will remove
There was a problem hiding this comment.
If we create session session = Session.getInstance(prop, null); with null authentication like this, will it still throw an error?
There was a problem hiding this comment.
#37319 (comment) As per this, if we set auth to false, we wont require this isAuthRequired, let's please remove that
| assertEquals( | ||
| Set.of(new AppsmithPluginException(AppsmithPluginError.PLUGIN_AUTHENTICATION_ERROR).getMessage()), | ||
| pluginExecutor.validateDatasource(invalidDatasourceConfiguration)); | ||
| assertEquals(0, pluginExecutor.validateDatasource(noAuthDatasourceConfiguration).size()); |
There was a problem hiding this comment.
In this test case file, we are creating docker container for intergration testing, can you check if we can configure this without username and password and see if we can write test case for it? This way we would have tested the smtp without credentials end to end
There was a problem hiding this comment.
will add one more test case for that
… more testcase - removed unwanted checks
|
@sneha122 Please review the changes |
app/server/appsmith-plugins/smtpPlugin/src/test/java/com/external/plugins/SmtpPluginTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-plugins/smtpPlugin/src/test/java/com/external/plugins/SmtpPluginTest.java (1)
67-69: Consider adding container validation.The connection setup could benefit from validating that the container is running before accessing its properties.
private static void configureSmtpConnection(GenericContainer container) { + if (!container.isRunning()) { + throw new IllegalStateException("SMTP container is not running"); + } host = container.getContainerIpAddress(); port = Long.valueOf(container.getFirstMappedPort()); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/server/appsmith-plugins/smtpPlugin/src/test/java/com/external/plugins/SmtpPluginTest.java(3 hunks)
🔇 Additional comments (3)
app/server/appsmith-plugins/smtpPlugin/src/test/java/com/external/plugins/SmtpPluginTest.java (3)
54-57: LGTM: Well-configured test container for unauthenticated SMTP.
The container is properly configured with empty credentials and a different port (1025) to avoid conflicts with the authenticated container.
63-69: Good refactoring of the connection setup.
The configureSmtpConnection method nicely encapsulates the container configuration logic, making it reusable for both auth and no-auth scenarios.
135-149: Comprehensive test for unauthenticated SMTP connection.
The test properly verifies that:
- Connection succeeds without authentication
- No validation errors are reported
- Test environment is restored after execution
This aligns well with the PR objective to support SMTP without credentials.
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
| import java.util.*; |
There was a problem hiding this comment.
@shanid544 Nit: Can we please keep the imports as is and not make it wildcard imports like java.util.*
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
| import static org.junit.jupiter.api.Assertions.*; |
There was a problem hiding this comment.
Same here, let's keep the imports as is
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-plugins/smtpPlugin/src/test/java/com/external/plugins/SmtpPluginTest.java (1)
30-37: Consider using specific imports instead of wildcardsThe imports could be more specific to improve code clarity and prevent potential naming conflicts.
-import java.util.List; -import java.util.Map; -import java.util.Arrays; -import java.util.HashMap; -import java.util.Set; -import java.util.ArrayList; -import java.util.stream.Collectors; +import java.util.List; +import java.util.Map; +import java.util.Arrays; +import java.util.HashMap; +import java.util.Set; +import java.util.ArrayList; +import java.util.stream.Collectors;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/server/appsmith-plugins/smtpPlugin/src/test/java/com/external/plugins/SmtpPluginTest.java(3 hunks)
🔇 Additional comments (3)
app/server/appsmith-plugins/smtpPlugin/src/test/java/com/external/plugins/SmtpPluginTest.java (3)
62-65: LGTM! Well-configured container for testing unauthenticated SMTP
The container is properly configured with:
- Custom port to avoid conflicts
- Empty credentials
- Appropriate command parameters
71-77: LGTM! Good refactoring of connection configuration
The helper method improves code reusability and makes container switching cleaner.
143-157: LGTM! Comprehensive test for unauthenticated SMTP
The test:
- Properly switches to unauthenticated container
- Verifies successful connection without credentials
- Restores original configuration
- Follows existing test patterns
|
Thank you @shanid544 PR looks good to me, ill create shadow PR and run test cases on it |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11833072143. |
|
Deploy-Preview-URL: https://ce-37319.dp.appsmith.com |
|
@sneha122 Could you please approve this. |
shanid544
changed the title
mohanarpit
changed the title
|
@sneha122 2 check are pending, those should automatically trigger ryt? |
|
@shanid544 Can you please run this command on server |
|
Created shadow Pr here for monitoring tests: #37393 |
@sneha122 Ran, should I commit that changes as well? |
yes please |
|
@sneha122 Formatted |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/server/appsmith-plugins/smtpPlugin/src/test/java/com/external/plugins/SmtpPluginTest.java (2)
61-65: Consider extracting container configuration to constantsThe container configuration could be more maintainable by extracting the port and command as constants.
+ private static final int SMTP_NO_AUTH_PORT = 1025; + private static final String SMTP_NO_AUTH_COMMAND = + "bin/maildev --base-pathname /maildev --smtp-port " + SMTP_NO_AUTH_PORT + " --incoming-user '' --incoming-pass ''"; + @Container public static final GenericContainer smtpWithoutAuth = new GenericContainer( DockerImageName.parse("maildev/maildev")) - .withExposedPorts(1025) - .withCommand("bin/maildev --base-pathname /maildev --smtp-port 1025 --incoming-user '' --incoming-pass ''"); + .withExposedPorts(SMTP_NO_AUTH_PORT) + .withCommand(SMTP_NO_AUTH_COMMAND);
143-157: Add JavaDoc documentation for the test methodThe test method is well-structured, but adding JavaDoc would improve clarity by documenting the test's purpose and expectations.
+ /** + * Tests that SMTP connection succeeds when authentication is not required. + * Verifies that the datasource can be configured without username and password. + */ @Test public void testConnectionWithoutAuth() {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/server/appsmith-plugins/smtpPlugin/src/main/java/com/external/plugins/SmtpPlugin.java(2 hunks)app/server/appsmith-plugins/smtpPlugin/src/test/java/com/external/plugins/SmtpPluginTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-plugins/smtpPlugin/src/main/java/com/external/plugins/SmtpPlugin.java
🔇 Additional comments (1)
app/server/appsmith-plugins/smtpPlugin/src/test/java/com/external/plugins/SmtpPluginTest.java (1)
71-77: LGTM! Well-structured setup method
The refactored setup with configureSmtpConnection method improves code reusability and maintainability by centralizing the configuration logic.
|
Thanks @shanid544 Once all tests are completed, will merge the PR |
…ithorg#37319) ## Description ### Bug Description Issue: When configuring an SMTP datasource without a username and password, the system throws an error: “Invalid authentication credentials. Please check datasource configuration.” Expected Behavior: The system should allow SMTP datasource configuration without requiring authentication credentials, as it is possible to connect to some SMTP servers without a username or password. **Steps To Reproduce** Use any email service for configuring SMTP datasource without setting up username and password Create SMTP datasource with host port, keeping username and password blank Test the configuration **Root Cause** Manual validation in the codebase was enforcing that both username and password fields are mandatory for SMTP configuration. This validation prevented the successful configuration of SMTP services that do not require authentication. **Solution Details** _To fix this, the following changes were implemented: Updated Validation Method (validateDatasource)_ Before: Enforced mandatory username and password validation. After: Removed the strict validation check for authentication fields, allowing for configurations without credentials. ``` if (authentication == null || !StringUtils.hasText(authentication.getUsername()) || !StringUtils.hasText(authentication.getPassword())) { invalids.add(new AppsmithPluginException(AppsmithPluginError.PLUGIN_AUTHENTICATION_ERROR).getMessage()); } ``` _Modified the SMTP Session Creation (datasourceCreate)_ Before: Always initialized the SMTP session with authentication, assuming credentials were required. After: Updated the session creation to support both authenticated and unauthenticated configurations. ``` Properties prop = new Properties(); prop.put("mail.smtp.auth", "false"); // Default to no authentication if (authentication != null && StringUtils.hasText(authentication.getUsername()) && StringUtils.hasText(authentication.getPassword())) { prop.put("mail.smtp.auth", "true"); session = Session.getInstance(prop, new Authenticator() { @OverRide protected PasswordAuthentication getPasswordAuthentication() { return new PasswordAuthentication(username, password); } }); } else { session = Session.getInstance(prop); } ``` _Enhanced Testing for Authentication Scenarios (testDatasource)_ Before: Errors were logged if authentication failed, even for servers where authentication wasn’t required. After: Introduced a flag to detect if authentication was required based on the session configuration, and adjusted error handling accordingly. ``` boolean isAuthRequired = "true".equals(connection.getProperty("mail.smtp.auth")); if (isAuthRequired && transport != null) { try { transport.connect(); } catch (AuthenticationFailedException e) { invalids.add(SMTPErrorMessages.DS_AUTHENTICATION_FAILED_ERROR_MSG); } } ``` **Testing and Verification** **Unit Tests** Without Authentication: _Updated testNullAuthentication test case to ensure no errors are returned when authentication is absent._ ``` @test public void testNullAuthentication() { DatasourceConfiguration invalidDatasourceConfiguration = createDatasourceConfiguration(); invalidDatasourceConfiguration.setAuthentication(null); assertEquals(0, pluginExecutor.validateDatasource(invalidDatasourceConfiguration).size()); } ``` Fixes appsmithorg#37271 ## Automation /ok-to-test tags="" updated testNullAuthentication() from SmtpPluginTest class <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced SMTP plugin now supports conditional authentication during session creation. - Improved error handling for authentication failures, providing clearer validation results. - Added support for testing SMTP connections without authentication. - **Bug Fixes** - Streamlined validation logic for datasource configurations, particularly for authentication scenarios. - **Documentation** - Updated test methods to clarify expected error messages for invalid configurations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: muhammed.shanid@zemosolabs.com <muhammed.shanid@zemosolabs.com>
…provided (#40005) ## Background [This PR](#37319) updated the SMTP datasource in Appsmith to support configurations without requiring a username and password. However, it inadvertently enforced the `mail.smtp.starttls.enable` property even when credentials were not provided. This led to an issue where STARTTLS was unnecessarily enforced, causing problems for users configuring SMTP without authentication, as detailed in [this issue](#39965). ## Description This PR resolves the issue by ensuring that the starttls and auth properties are only set when a username and password are provided. This change allows the SMTP datasource to function correctly without authentication, aligning with user expectations and preventing unnecessary STARTTLS enforcement. Fixes #39965 ## Automation /ok-to-test tags="@tag.Datasource" ### 🔍 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/14192755822> > Commit: 7334f48 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=14192755822&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Datasource` > Spec: > <hr>Tue, 01 Apr 2025 10:42:30 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved the email configuration experience by ensuring that security settings for authentication and encrypted connections are applied only when valid credentials are provided, resulting in more reliable behavior for both authenticated and non-authenticated email server setups. - Enhanced validation in tests to confirm that session properties reflect the correct settings based on authentication presence. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…provided (appsmithorg#40005) ## Background [This PR](appsmithorg#37319) updated the SMTP datasource in Appsmith to support configurations without requiring a username and password. However, it inadvertently enforced the `mail.smtp.starttls.enable` property even when credentials were not provided. This led to an issue where STARTTLS was unnecessarily enforced, causing problems for users configuring SMTP without authentication, as detailed in [this issue](appsmithorg#39965). ## Description This PR resolves the issue by ensuring that the starttls and auth properties are only set when a username and password are provided. This change allows the SMTP datasource to function correctly without authentication, aligning with user expectations and preventing unnecessary STARTTLS enforcement. Fixes appsmithorg#39965 ## Automation /ok-to-test tags="@tag.Datasource" ### 🔍 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/14192755822> > Commit: 7334f48 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=14192755822&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Datasource` > Spec: > <hr>Tue, 01 Apr 2025 10:42:30 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved the email configuration experience by ensuring that security settings for authentication and encrypted connections are applied only when valid credentials are provided, resulting in more reliable behavior for both authenticated and non-authenticated email server setups. - Enhanced validation in tests to confirm that session properties reflect the correct settings based on authentication presence. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
4 participants
Description
Bug Description
Issue: When configuring an SMTP datasource without a username and password, the system throws an error: “Invalid authentication credentials. Please check datasource configuration.”
Expected Behavior: The system should allow SMTP datasource configuration without requiring authentication credentials, as it is possible to connect to some SMTP servers without a username or password.
Steps To Reproduce
Use any email service for configuring SMTP datasource without setting up username and password
Create SMTP datasource with host port, keeping username and password blank
Test the configuration
Root Cause
Manual validation in the codebase was enforcing that both username and password fields are mandatory for SMTP configuration. This validation prevented the successful configuration of SMTP services that do not require authentication.
Solution Details
To fix this, the following changes were implemented:
Updated Validation Method (validateDatasource)
Before: Enforced mandatory username and password validation.
After: Removed the strict validation check for authentication fields, allowing for configurations without credentials.
Modified the SMTP Session Creation (datasourceCreate)
Before: Always initialized the SMTP session with authentication, assuming credentials were required.
After: Updated the session creation to support both authenticated and unauthenticated configurations.
Enhanced Testing for Authentication Scenarios (testDatasource)
Before: Errors were logged if authentication failed, even for servers where authentication wasn’t required.
After: Introduced a flag to detect if authentication was required based on the session configuration, and adjusted error handling accordingly.
Testing and Verification
Unit Tests
Without Authentication:
Updated testNullAuthentication test case to ensure no errors are returned when authentication is absent.
Fixes #37271
Automation
/ok-to-test tags=""
updated testNullAuthentication() from SmtpPluginTest class
Summary by CodeRabbit
New Features
Bug Fixes
Documentation