Skip to content

Commit

Permalink
fix: SMTP datasource should work without username and password (#37319)
Browse files Browse the repository at this point in the history
## 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 #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>
  • Loading branch information
shanid544 and shanidmuhammedzemoso authored Nov 15, 2024
1 parent 3e67be8 commit 6fc6327
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -212,15 +212,25 @@ public Mono<Session> datasourceCreate(DatasourceConfiguration datasourceConfigur
prop.put("mail.smtp.port", String.valueOf(port));
prop.put("mail.smtp.ssl.trust", endpoint.getHost());

String username = authentication.getUsername();
String password = authentication.getPassword();
Session session;

Session session = Session.getInstance(prop, new Authenticator() {
@Override
protected PasswordAuthentication getPasswordAuthentication() {
return new PasswordAuthentication(username, password);
}
});
if (authentication != null
&& StringUtils.hasText(authentication.getUsername())
&& StringUtils.hasText(authentication.getPassword())) {

String username = authentication.getUsername();
String password = authentication.getPassword();

session = Session.getInstance(prop, new Authenticator() {
@Override
protected PasswordAuthentication getPasswordAuthentication() {
return new PasswordAuthentication(username, password);
}
});
} else {
prop.put("mail.smtp.auth", "false");
session = Session.getInstance(prop);
}
return Mono.just(session);
}

Expand Down Expand Up @@ -248,14 +258,6 @@ public Set<String> validateDatasource(DatasourceConfiguration datasourceConfigur
invalids.add(SMTPErrorMessages.DS_MISSING_HOST_ADDRESS_ERROR_MSG);
}
}

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());
}

return invalids;
}

Expand All @@ -264,6 +266,7 @@ public Mono<DatasourceTestResult> testDatasource(Session connection) {
log.debug(Thread.currentThread().getName() + ": testDatasource() called for SMTP plugin.");
return Mono.fromCallable(() -> {
Set<String> invalids = new HashSet<>();

try {
Transport transport = connection.getTransport();
if (transport != null) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package com.external.plugins;

import com.appsmith.external.exceptions.pluginExceptions.AppsmithPluginError;
import com.appsmith.external.exceptions.pluginExceptions.AppsmithPluginException;
import com.appsmith.external.helpers.PluginUtils;
import com.appsmith.external.models.ActionConfiguration;
Expand Down Expand Up @@ -59,12 +58,23 @@ public class SmtpPluginTest {
.withCommand("bin/maildev --base-pathname /maildev --incoming-user " + username + " --incoming-pass "
+ password + " -s 25");

@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 ''");

private final SmtpPlugin.SmtpPluginExecutor pluginExecutor = new SmtpPlugin.SmtpPluginExecutor();

@BeforeAll
public static void setup() {
host = smtp.getContainerIpAddress();
port = Long.valueOf(smtp.getFirstMappedPort());
// Initialize SMTP connection with default configuration (can be changed per test)
configureSmtpConnection(smtp); // Default
}

private static void configureSmtpConnection(GenericContainer container) {
host = container.getContainerIpAddress();
port = Long.valueOf(container.getFirstMappedPort());
}

private DatasourceConfiguration createDatasourceConfiguration() {
Expand Down Expand Up @@ -130,13 +140,21 @@ public void testInvalidPort() {
}

@Test
public void testNullAuthentication() {
DatasourceConfiguration invalidDatasourceConfiguration = createDatasourceConfiguration();
invalidDatasourceConfiguration.setAuthentication(null);
public void testConnectionWithoutAuth() {
configureSmtpConnection(smtpWithoutAuth);
DatasourceConfiguration noAuthDatasourceConfiguration = createDatasourceConfiguration();
noAuthDatasourceConfiguration.setAuthentication(null); // No authentication

assertEquals(
Set.of(new AppsmithPluginException(AppsmithPluginError.PLUGIN_AUTHENTICATION_ERROR).getMessage()),
pluginExecutor.validateDatasource(invalidDatasourceConfiguration));
Mono<DatasourceTestResult> testDatasourceMono = pluginExecutor.testDatasource(noAuthDatasourceConfiguration);

StepVerifier.create(testDatasourceMono)
.assertNext(datasourceTestResult -> {
assertNotNull(datasourceTestResult);
assertTrue(datasourceTestResult.isSuccess());
assertTrue(datasourceTestResult.getInvalids().isEmpty());
})
.verifyComplete();
configureSmtpConnection(smtp);
}

@Test
Expand Down

0 comments on commit 6fc6327

Please sign in to comment.