MX 241: harden SMTP fallback and notification delivery reliability#150
Merged
IOhacker merged 1 commit intoopenMF:developfrom Apr 15, 2026
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 47 minutes and 46 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughCentralizes SMTP credential resolution with DB-first and Spring-property fallback, adds SmtpConfigurationUnavailableException, adjusts notification event null-safety and cooldown logic, refactors notification sending and SMS provider selection, updates templates for null-safe defaults, and adds unit and integration tests covering fallback and logging behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant NSvc as SelfServiceNotificationService
participant SSvc as SelfServicePluginEmailService
participant DB as Database
participant Env as Spring\ Environment
participant Mail as JavaMailSender
NSvc->>SSvc: sendFormattedEmail(event, template, ctx)
SSvc->>SSvc: resolveSmtpCredentials()
rect rgba(100,149,237,0.5)
SSvc->>DB: externalServicesReadPlatformService.getSMTPCredentials()
alt DB returns credentials
DB-->>SSvc: SMTPCredentialsData
else DB throws DataAccessException
DB--x SSvc: Exception
SSvc->>Env: Read fineract.selfservice.smtp.* properties
Env-->>SSvc: env properties
SSvc->>SSvc: validate host & from-email
alt required fields present
SSvc-->>SSvc: build SMTPCredentialsData
else missing required fields
SSvc-->>NSvc: throw SmtpConfigurationUnavailableException
end
end
end
alt SMTP configured
SSvc->>SSvc: configureMailSender(SMTPCredentialsData)
SSvc->>Mail: send(message)
Mail-->>SSvc: send result
SSvc-->>NSvc: return/send status
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/main/java/org/apache/fineract/selfservice/security/api/SelfAuthenticationApiResource.java (1)
251-253: GuardX-Forwarded-Forusage behind a trusted-proxy boundary.At Line 252-Line 253, the header is accepted unconditionally. A direct client can spoof it, which can corrupt notification/audit IP data. Only use forwarded headers when the request passed through trusted proxy infrastructure.
As per coding guidelines, "Validate and sanitize all incoming data (use Bean Validation with
@Validand constraints)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/selfservice/security/api/SelfAuthenticationApiResource.java` around lines 251 - 253, The code unconditionally trusts the X-Forwarded-For header in SelfAuthenticationApiResource; restrict use to requests that came through configured trusted proxies (e.g., check remote address against a trusted-proxies config or a helper like isTrustedProxy(remoteAddr)) and only then parse X-Forwarded-For. When using the header, split and trim the first entry, validate/sanitize it using a proper IP parse (e.g., InetAddress resolution) and catch parse exceptions; if validation fails or the request is not from a trusted proxy, fall back to httpRequest.getRemoteAddr(). Ensure any config-driven trusted-proxies list is consulted and validated before trusting the header.src/main/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationService.java (1)
99-99: Use import statement for@Transactionalannotation.Other annotations in this class (
@Slf4j,@Service,@Async,@EventListener) use imports. The fully qualified@org.springframework.transaction.annotation.Transactionalis inconsistent.Proposed fix
Add to imports:
import org.springframework.transaction.annotation.Transactional;Then change line 99:
-@org.springframework.transaction.annotation.Transactional +@Transactional🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationService.java` at line 99, Replace the fully-qualified annotation usage with an import: add "import org.springframework.transaction.annotation.Transactional;" to the imports and change the annotation on SelfServiceNotificationService from "@org.springframework.transaction.annotation.Transactional" to "@Transactional" to match the style of other annotations like `@Slf4j`, `@Service`, `@Async`, and `@EventListener`.src/main/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailService.java (2)
64-68: Addfinalmodifier to theenvparameter for consistency.The first constructor parameter has
finalbutenvdoes not. Per coding guidelines, constructor-injected dependencies should be markedfinal.Proposed fix
`@Autowired` public SelfServicePluginEmailService(final ExternalServicesPropertiesReadPlatformService externalServicesReadPlatformService, - org.springframework.core.env.Environment env) { + final org.springframework.core.env.Environment env) { this.externalServicesReadPlatformService = externalServicesReadPlatformService; this.env = env; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailService.java` around lines 64 - 68, Constructor parameter consistency: mark the env parameter as final in the SelfServicePluginEmailService constructor to match the first parameter and coding guidelines; update the constructor signature (public SelfServicePluginEmailService(final ExternalServicesPropertiesReadPlatformService externalServicesReadPlatformService, final org.springframework.core.env.Environment env)) so env is declared final and no other changes are needed.
224-229: Simplify TLS configuration to use STARTTLS only.Combining
mail.smtp.starttls.enable=truewith explicit SSLSocketFactory is not aligned with modern JavaMail best practices. Official documentation indicates SSLSocketFactory is rarely needed. Use STARTTLS properties alone:Proposed fix
if (smtpCredentialsData.isUseTLS()) { props.put("mail.smtp.starttls.enable", "true"); - props.put("mail.smtp.socketFactory.port", Integer.toString(port)); - props.put("mail.smtp.socketFactory.class", "javax.net.ssl.SSLSocketFactory"); // NOSONAR - props.put("mail.smtp.socketFactory.fallback", "true"); + props.put("mail.smtp.starttls.required", "true"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailService.java` around lines 224 - 229, In the smtpCredentialsData.isUseTLS() block inside SelfServicePluginEmailService, remove the explicit SSL socket factory properties ("mail.smtp.socketFactory.port", "mail.smtp.socketFactory.class", "mail.smtp.socketFactory.fallback") and leave only the STARTTLS setting (e.g., set "mail.smtp.starttls.enable" to "true"; optionally set "mail.smtp.starttls.required" if you want to enforce it) so the code uses STARTTLS per JavaMail best practices instead of forcing an SSLSocketFactory.src/test/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationServiceTest.java (1)
79-83: Consider using imports instead of fully qualified class names.Fully qualified class names for
ThreadLocalContextUtil,FineractPlatformTenant,BusinessDateType, andLocalDatemake the code verbose. Adding explicit imports would improve readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationServiceTest.java` around lines 79 - 83, Replace the verbose fully-qualified usages with imports: add import statements for ThreadLocalContextUtil, FineractPlatformTenant, BusinessDateType and LocalDate at the top of the test file and then update the occurrences (the ThreadLocalContextUtil.setTenant call, new FineractPlatformTenant(...), BusinessDateType.BUSINESS_DATE, and java.time.LocalDate.now()) to use the short class names; this will remove the long package prefixes and improve readability while keeping behavior unchanged.src/test/java/org/apache/fineract/selfservice/notification/SelfServiceSmtpFallbackIntegrationTest.java (1)
271-286: Completion listener relies on sleep-based ordering.The 100ms sleep to "ensure the primary handler runs first" is timing-dependent and may be flaky under high load. Consider using a more deterministic synchronization mechanism.
However, this is acceptable for integration tests where exact ordering is less critical than completion detection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/apache/fineract/selfservice/notification/SelfServiceSmtpFallbackIntegrationTest.java` around lines 271 - 286, The onNotification method currently uses Thread.sleep(100) to try to order handlers which is flaky; instead create a deterministic synchronization primitive (e.g., a CountDownLatch or CompletableFuture) named something like primaryHandlerDone and have the primary notification handler signal primaryHandlerDone when it finishes; then in this fallback/onNotification listener wait on primaryHandlerDone (with a timeout) before calling latch.countDown(). Also update awaitCompletion if needed to account for the new signal, and reference the `@Async`("notificationExecutor") listener method onNotification, the existing latch, and the SelfServiceNotificationEvent to locate where to add/wait on the primaryHandlerDone signal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationService.java`:
- Around line 189-197: The SMS sending block saves messages as PENDING but never
marks them SENT on success; after calling
smsScheduledJobService.sendTriggeredMessage(...) successfully, set
smsMessage.setStatusType(SmsMessageStatusType.SENT.getValue()) and persist via
smsMessageRepository.save(smsMessage); apply the same fix in
SelfServiceRegistrationWritePlatformServiceImpl and
SelfServiceForgotPasswordWritePlatformServiceImpl, and ensure any asynchronous
gateway/webhook handlers can still override the SENT state if they provide later
delivery/failure updates.
In
`@src/main/java/org/apache/fineract/selfservice/notification/starter/SelfServiceNotificationConfig.java`:
- Around line 194-206: The NotificationExecutorProperties class annotated with
`@ConfigurationProperties` contains `@jakarta.validation.constraints.Min`
annotations but lacks `@Validated` so constraints won't be enforced; add the
Spring validation annotation
(org.springframework.validation.annotation.Validated) to the
NotificationExecutorProperties class declaration (keeping the existing
`@ConfigurationProperties`, `@lombok.Getter` and `@lombok.Setter`) so Spring Boot will
apply the Min constraints during binding, and add the corresponding import.
In
`@src/main/java/org/apache/fineract/selfservice/security/api/SelfAuthenticationApiResource.java`:
- Around line 251-255: The extracted X-Forwarded-For token can be empty (e.g. ",
10.0.0.1"); update the logic in SelfAuthenticationApiResource where
xForwardedFor is processed: after getting xForwardedFor from httpRequest, split
on "," and take the first token, trim it, then check
StringUtils.isNotBlank(firstToken) before returning it; if the trimmed
firstToken is blank, fall back to httpRequest.getRemoteAddr(). Ensure you
reference the xForwardedFor variable and httpRequest.getRemoteAddr() in the fix.
In
`@src/test/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationSmtpIntegrationTest.java`:
- Around line 200-207: The current assertion in
SelfServiceNotificationSmtpIntegrationTest that checks health.getStatusCode() <
600 is too permissive; replace it with a stricter check that the health response
is a successful HTTP status (e.g., verify status is 200 or in the 2xx range).
Locate the variable health and the call to
get("/fineract-provider/actuator/health") and change the assertion to assert
that health.getStatusCode() is between 200 and 299 (or equals 200) and keep the
existing message, so the test fails on 5xx server errors rather than passing
them.
---
Nitpick comments:
In
`@src/main/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailService.java`:
- Around line 64-68: Constructor parameter consistency: mark the env parameter
as final in the SelfServicePluginEmailService constructor to match the first
parameter and coding guidelines; update the constructor signature (public
SelfServicePluginEmailService(final
ExternalServicesPropertiesReadPlatformService
externalServicesReadPlatformService, final
org.springframework.core.env.Environment env)) so env is declared final and no
other changes are needed.
- Around line 224-229: In the smtpCredentialsData.isUseTLS() block inside
SelfServicePluginEmailService, remove the explicit SSL socket factory properties
("mail.smtp.socketFactory.port", "mail.smtp.socketFactory.class",
"mail.smtp.socketFactory.fallback") and leave only the STARTTLS setting (e.g.,
set "mail.smtp.starttls.enable" to "true"; optionally set
"mail.smtp.starttls.required" if you want to enforce it) so the code uses
STARTTLS per JavaMail best practices instead of forcing an SSLSocketFactory.
In
`@src/main/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationService.java`:
- Line 99: Replace the fully-qualified annotation usage with an import: add
"import org.springframework.transaction.annotation.Transactional;" to the
imports and change the annotation on SelfServiceNotificationService from
"@org.springframework.transaction.annotation.Transactional" to "@Transactional"
to match the style of other annotations like `@Slf4j`, `@Service`, `@Async`, and
`@EventListener`.
In
`@src/main/java/org/apache/fineract/selfservice/security/api/SelfAuthenticationApiResource.java`:
- Around line 251-253: The code unconditionally trusts the X-Forwarded-For
header in SelfAuthenticationApiResource; restrict use to requests that came
through configured trusted proxies (e.g., check remote address against a
trusted-proxies config or a helper like isTrustedProxy(remoteAddr)) and only
then parse X-Forwarded-For. When using the header, split and trim the first
entry, validate/sanitize it using a proper IP parse (e.g., InetAddress
resolution) and catch parse exceptions; if validation fails or the request is
not from a trusted proxy, fall back to httpRequest.getRemoteAddr(). Ensure any
config-driven trusted-proxies list is consulted and validated before trusting
the header.
In
`@src/test/java/org/apache/fineract/selfservice/notification/SelfServiceSmtpFallbackIntegrationTest.java`:
- Around line 271-286: The onNotification method currently uses
Thread.sleep(100) to try to order handlers which is flaky; instead create a
deterministic synchronization primitive (e.g., a CountDownLatch or
CompletableFuture) named something like primaryHandlerDone and have the primary
notification handler signal primaryHandlerDone when it finishes; then in this
fallback/onNotification listener wait on primaryHandlerDone (with a timeout)
before calling latch.countDown(). Also update awaitCompletion if needed to
account for the new signal, and reference the `@Async`("notificationExecutor")
listener method onNotification, the existing latch, and the
SelfServiceNotificationEvent to locate where to add/wait on the
primaryHandlerDone signal.
In
`@src/test/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationServiceTest.java`:
- Around line 79-83: Replace the verbose fully-qualified usages with imports:
add import statements for ThreadLocalContextUtil, FineractPlatformTenant,
BusinessDateType and LocalDate at the top of the test file and then update the
occurrences (the ThreadLocalContextUtil.setTenant call, new
FineractPlatformTenant(...), BusinessDateType.BUSINESS_DATE, and
java.time.LocalDate.now()) to use the short class names; this will remove the
long package prefixes and improve readability while keeping behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dd1d6e3c-32a2-4a04-8401-2978acc14627
📒 Files selected for processing (16)
src/main/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailService.javasrc/main/java/org/apache/fineract/infrastructure/core/service/SmtpConfigurationUnavailableException.javasrc/main/java/org/apache/fineract/selfservice/notification/NotificationCooldownCache.javasrc/main/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationEvent.javasrc/main/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationService.javasrc/main/java/org/apache/fineract/selfservice/notification/starter/SelfServiceNotificationConfig.javasrc/main/java/org/apache/fineract/selfservice/security/api/SelfAuthenticationApiResource.javasrc/main/resources/notification/templates/html/login-failure.htmlsrc/main/resources/notification/templates/html/login-success.htmlsrc/main/resources/notification/templates/text/login-failure.txtsrc/main/resources/notification/templates/text/login-success.txtsrc/test/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailServiceTest.javasrc/test/java/org/apache/fineract/selfservice/notification/NotificationCooldownCacheTest.javasrc/test/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationSmtpIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/notification/SelfServiceSmtpFallbackIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationServiceTest.java
💤 Files with no reviewable changes (1)
- src/main/java/org/apache/fineract/selfservice/notification/NotificationCooldownCache.java
DeathGun44
force-pushed
the
MX-241-harden-notification-smtp-fallback
branch
from
c55cb5b to
8d61dbc
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/main/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailService.java (1)
63-68: Prefer Lombok constructor injection here.This manual DI constructor adds boilerplate and is still undocumented even though it is public.
@RequiredArgsConstructorwould align with the project standard and remove the need to maintain this constructor by hand.As per coding guidelines, "Use RequiredArgsConstructor for dependency injection" and "Public methods and classes MUST have Javadoc."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailService.java` around lines 63 - 68, Replace the handwritten public constructor in SelfServicePluginEmailService with Lombok-based constructor injection: mark the class with `@RequiredArgsConstructor`, make the injected fields (externalServicesReadPlatformService and env) final, remove the explicit constructor, and add a Javadoc comment on the class (and any remaining public methods) per guidelines; ensure lombok.RequiredArgsConstructor is imported and the class-level Javadoc describes the service responsibility.src/main/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationService.java (1)
70-83: Prefer@RequiredArgsConstructorfor this bean.The explicit DI constructor adds boilerplate and is public without Javadoc. Lombok constructor injection would match the project standard and keep this service easier to maintain.
As per coding guidelines, "Use RequiredArgsConstructor for dependency injection" and "Public methods and classes MUST have Javadoc."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationService.java` around lines 70 - 83, Replace the explicit public constructor SelfServiceNotificationService(...) with Lombok constructor injection: mark the class SelfServiceNotificationService with `@RequiredArgsConstructor`, make all injected fields final (notificationTemplateEngine, notificationMessageSource, emailService, smsMessageRepository, smsScheduledJobService, smsProviderService, notificationCooldownCache, env), and remove the manual constructor; also add the required Javadoc for the public class (and any public methods) to comply with project guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailService.java`:
- Around line 155-166: resolveSmtpCredentials() must not propagate
SmtpConfigurationUnavailableException because other callers of
sendToUserAccount()/sendDefinedEmail() expect email failures to degrade
gracefully; change resolveSmtpCredentials() so it catches
SmtpConfigurationUnavailableException (in addition to DataAccessException), log
exactly as the current DataAccessException path using smtpFallbackLogged and
then return buildCredentialsFromEnvironment(exception) instead of letting the
SmtpConfigurationUnavailableException escape; this keeps existing callers
(registration, forgot-password, user-creation) from rolling back while
SelfServiceNotificationService can still handle the specific exception if
desired.
- Around line 201-210: The port parsing currently only catches non-numeric
input; after parsing portStr via smtpCredentialsData.getPort() into port,
validate that port is within the valid TCP/UDP range (1–65535); if parsed value
is out of range (<=0 or >65535) treat it as invalid, log a warning similar to
the NumberFormatException case (including portStr), and fall back to the default
587 before calling mailSender.setPort(port). Ensure you still catch
NumberFormatException from Integer.parseInt and handle both cases consistently.
In
`@src/main/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationService.java`:
- Around line 145-149: The cooldown entry acquired in handleNotification is not
released when delivery is skipped, so update sendEmailNotification (and the
analogous early-return paths indicated around lines 159-166 and 182-184) to call
the cooldown-release/invalidator before every early return: locate the method
that acquires the cooldown in handleNotification and use its corresponding
release method (e.g., releaseCooldown(event) or cooldownCache.invalidate(key))
immediately prior to logging and returning from sendEmailNotification and the
other skip branches (missing email/sms or missing provider config) so the cache
entry is removed if no delivery attempt is made.
- Around line 150-155: The code builds an HTML template ("html/...") via
notificationTemplateEngine.process but sends it with
emailService.sendDefinedEmail which uses SimpleMailMessage (plain text), causing
raw HTML to be received; update SelfServiceNotificationService to send the
rendered HTML using the email service's MIME/HTML-capable API (e.g., call
emailService.sendHtmlEmail or a sendMimeMessage variant) or set the EmailDetail
to indicate HTML content if the service supports it, replacing
sendDefinedEmail(...) with the HTML/MIME send method so the HTML body is
delivered as formatted mail.
---
Nitpick comments:
In
`@src/main/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailService.java`:
- Around line 63-68: Replace the handwritten public constructor in
SelfServicePluginEmailService with Lombok-based constructor injection: mark the
class with `@RequiredArgsConstructor`, make the injected fields
(externalServicesReadPlatformService and env) final, remove the explicit
constructor, and add a Javadoc comment on the class (and any remaining public
methods) per guidelines; ensure lombok.RequiredArgsConstructor is imported and
the class-level Javadoc describes the service responsibility.
In
`@src/main/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationService.java`:
- Around line 70-83: Replace the explicit public constructor
SelfServiceNotificationService(...) with Lombok constructor injection: mark the
class SelfServiceNotificationService with `@RequiredArgsConstructor`, make all
injected fields final (notificationTemplateEngine, notificationMessageSource,
emailService, smsMessageRepository, smsScheduledJobService, smsProviderService,
notificationCooldownCache, env), and remove the manual constructor; also add the
required Javadoc for the public class (and any public methods) to comply with
project guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5219ff87-cce1-4bf1-bb1e-cb6711cb4908
📒 Files selected for processing (16)
src/main/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailService.javasrc/main/java/org/apache/fineract/infrastructure/core/service/SmtpConfigurationUnavailableException.javasrc/main/java/org/apache/fineract/selfservice/notification/NotificationCooldownCache.javasrc/main/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationEvent.javasrc/main/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationService.javasrc/main/java/org/apache/fineract/selfservice/notification/starter/SelfServiceNotificationConfig.javasrc/main/java/org/apache/fineract/selfservice/security/api/SelfAuthenticationApiResource.javasrc/main/resources/notification/templates/html/login-failure.htmlsrc/main/resources/notification/templates/html/login-success.htmlsrc/main/resources/notification/templates/text/login-failure.txtsrc/main/resources/notification/templates/text/login-success.txtsrc/test/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailServiceTest.javasrc/test/java/org/apache/fineract/selfservice/notification/NotificationCooldownCacheTest.javasrc/test/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationSmtpIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/notification/SelfServiceSmtpFallbackIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationServiceTest.java
💤 Files with no reviewable changes (1)
- src/main/java/org/apache/fineract/selfservice/notification/NotificationCooldownCache.java
✅ Files skipped from review due to trivial changes (5)
- src/main/resources/notification/templates/text/login-failure.txt
- src/main/resources/notification/templates/html/login-failure.html
- src/main/java/org/apache/fineract/infrastructure/core/service/SmtpConfigurationUnavailableException.java
- src/test/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailServiceTest.java
- src/main/java/org/apache/fineract/selfservice/notification/starter/SelfServiceNotificationConfig.java
🚧 Files skipped from review as they are similar to previous changes (6)
- src/main/resources/notification/templates/html/login-success.html
- src/main/resources/notification/templates/text/login-success.txt
- src/test/java/org/apache/fineract/selfservice/notification/NotificationCooldownCacheTest.java
- src/test/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationSmtpIntegrationTest.java
- src/test/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationServiceTest.java
- src/test/java/org/apache/fineract/selfservice/notification/SelfServiceSmtpFallbackIntegrationTest.java
DeathGun44
force-pushed
the
MX-241-harden-notification-smtp-fallback
branch
from
8d61dbc to
9aedb92
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailService.java (1)
233-245: Consider conditional SMTP authentication based on credentials presence.
mail.smtp.authis unconditionally set to"true", but whenusernameis empty (which is allowed bybuildCredentialsFromEnvironment), some SMTP servers may reject the connection or behave unexpectedly. Consider enabling authentication only when credentials are provided.Proposed fix
Properties props = mailSender.getJavaMailProperties(); props.put("mail.transport.protocol", "smtp"); - props.put("mail.smtp.auth", "true"); + + String username = smtpCredentialsData.getUsername(); + if (username != null && !username.isEmpty()) { + props.put("mail.smtp.auth", "true"); + } else { + props.put("mail.smtp.auth", "false"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailService.java` around lines 233 - 245, The code currently unconditionally sets "mail.smtp.auth" to "true" in SelfServicePluginEmailService when configuring JavaMail properties; change this to enable SMTP auth only when credentials are present by checking smtpCredentialsData.getUsername() (or whatever accessor returns the configured username from buildCredentialsFromEnvironment) and only put("mail.smtp.auth","true") when username is non-empty/non-null, otherwise omit or set to "false"; keep the existing TLS handling (smtpCredentialsData.isUseTLS()) and mail.debug logic unchanged.src/main/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationService.java (1)
144-145: Potentially unreachable catch block.The catch block for
SmtpConfigurationUnavailableExceptionat line 144-145 appears unreachable.sendFormattedEmailwraps this exception inPlatformEmailSendExceptionbefore throwing (seeSelfServicePluginEmailServicelines 106-110), so the wrapped exception path at lines 136-143 handles this case.Consider removing the redundant catch block
} catch (org.apache.fineract.infrastructure.core.service.PlatformEmailSendException emailEx) { if (emailEx.getCause() instanceof SmtpConfigurationUnavailableException) { handleSmtpConfigError(event, (SmtpConfigurationUnavailableException) emailEx.getCause()); } else { String cacheKey = event.getType().name() + ":" + event.getUserId(); notificationCooldownCache.release(cacheKey); log.error("Failed to handle notification for event type {}", event.getType(), emailEx); } - } catch (SmtpConfigurationUnavailableException configEx) { - handleSmtpConfigError(event, configEx); } catch (Exception e) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationService.java` around lines 144 - 145, The catch block for SmtpConfigurationUnavailableException in SelfServiceNotificationService is redundant because sendFormattedEmail (implemented in SelfServicePluginEmailService) wraps that exception inside PlatformEmailSendException; either remove the specific SmtpConfigurationUnavailableException catch or change the exception handling to catch PlatformEmailSendException first and inspect/unwrap its cause to call handleSmtpConfigError when the cause is a SmtpConfigurationUnavailableException; update the code paths around sendFormattedEmail, PlatformEmailSendException, handleSmtpConfigError and handleEmailSendError accordingly so only the appropriate handler runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/test/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationServiceTest.java`:
- Around line 259-260: The mock currently throws
SmtpConfigurationUnavailableException directly from the
emailService.sendFormattedEmail stub in SelfServiceNotificationServiceTest,
causing a mismatch with expected behavior; change the mocked behavior to throw a
PlatformEmailSendException that wraps the SmtpConfigurationUnavailableException
(i.e., construct a new PlatformEmailSendException with the
SmtpConfigurationUnavailableException as its cause) when stubbing
emailService.sendFormattedEmail so the test aligns with how the production code
surfaces email errors.
- Around line 243-250: The test currently mocks emailService.sendFormattedEmail
to throw SmtpConfigurationUnavailableException directly, but in production
SelfServicePluginEmailService wraps that in PlatformEmailSendException; update
the mock in SelfServiceNotificationServiceTest so that doThrow(...) throws a
PlatformEmailSendException whose cause is a new
SmtpConfigurationUnavailableException("SMTP not configured") when
emailService.sendFormattedEmail(any(EmailDetail.class)) is invoked, then run the
same service.handleNotification(emailEvent()) and
verify(cooldownCache).release("LOGIN_SUCCESS:1") as before.
---
Nitpick comments:
In
`@src/main/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailService.java`:
- Around line 233-245: The code currently unconditionally sets "mail.smtp.auth"
to "true" in SelfServicePluginEmailService when configuring JavaMail properties;
change this to enable SMTP auth only when credentials are present by checking
smtpCredentialsData.getUsername() (or whatever accessor returns the configured
username from buildCredentialsFromEnvironment) and only
put("mail.smtp.auth","true") when username is non-empty/non-null, otherwise omit
or set to "false"; keep the existing TLS handling
(smtpCredentialsData.isUseTLS()) and mail.debug logic unchanged.
In
`@src/main/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationService.java`:
- Around line 144-145: The catch block for SmtpConfigurationUnavailableException
in SelfServiceNotificationService is redundant because sendFormattedEmail
(implemented in SelfServicePluginEmailService) wraps that exception inside
PlatformEmailSendException; either remove the specific
SmtpConfigurationUnavailableException catch or change the exception handling to
catch PlatformEmailSendException first and inspect/unwrap its cause to call
handleSmtpConfigError when the cause is a SmtpConfigurationUnavailableException;
update the code paths around sendFormattedEmail, PlatformEmailSendException,
handleSmtpConfigError and handleEmailSendError accordingly so only the
appropriate handler runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5472d5a3-69e1-4b40-b1a1-84095b5e84c3
📒 Files selected for processing (16)
src/main/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailService.javasrc/main/java/org/apache/fineract/infrastructure/core/service/SmtpConfigurationUnavailableException.javasrc/main/java/org/apache/fineract/selfservice/notification/NotificationCooldownCache.javasrc/main/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationEvent.javasrc/main/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationService.javasrc/main/java/org/apache/fineract/selfservice/notification/starter/SelfServiceNotificationConfig.javasrc/main/java/org/apache/fineract/selfservice/security/api/SelfAuthenticationApiResource.javasrc/main/resources/notification/templates/html/login-failure.htmlsrc/main/resources/notification/templates/html/login-success.htmlsrc/main/resources/notification/templates/text/login-failure.txtsrc/main/resources/notification/templates/text/login-success.txtsrc/test/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailServiceTest.javasrc/test/java/org/apache/fineract/selfservice/notification/NotificationCooldownCacheTest.javasrc/test/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationSmtpIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/notification/SelfServiceSmtpFallbackIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationServiceTest.java
💤 Files with no reviewable changes (1)
- src/main/java/org/apache/fineract/selfservice/notification/NotificationCooldownCache.java
✅ Files skipped from review due to trivial changes (6)
- src/main/resources/notification/templates/text/login-failure.txt
- src/main/resources/notification/templates/text/login-success.txt
- src/main/resources/notification/templates/html/login-failure.html
- src/main/java/org/apache/fineract/infrastructure/core/service/SmtpConfigurationUnavailableException.java
- src/main/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationEvent.java
- src/test/java/org/apache/fineract/selfservice/notification/SelfServiceSmtpFallbackIntegrationTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/resources/notification/templates/html/login-success.html
- src/main/java/org/apache/fineract/selfservice/security/api/SelfAuthenticationApiResource.java
- src/main/java/org/apache/fineract/selfservice/notification/starter/SelfServiceNotificationConfig.java
DeathGun44
force-pushed
the
MX-241-harden-notification-smtp-fallback
branch
from
9aedb92 to
71b8ae8
Compare
…iability - Add graceful DataAccessException handling in SelfServicePluginEmailService with DB → Spring properties fallback chain for SMTP credentials - Introduce SmtpConfigurationUnavailableException for clear error signaling - Release cooldown on config failure so retries work after fix is applied - Implement log-once WARN → DEBUG suppression for repeated SMTP errors - Respect useTLS flag instead of hardcoding TLS properties - Add try-catch for invalid SMTP port with fallback to default 587 - Parse X-Forwarded-For header for accurate client IP in notifications - Remove unused loggedInUser variable in SelfAuthenticationApiResource - Add constructor Javadoc + Objects.requireNonNull validation on event - Add safe non-PII toString() to SelfServiceNotificationEvent - Fix null-safe firstName/eventTimestamp in all notification templates - Add unauthorized login guidance to login-failure templates - Add <head>/charset to login-failure.html email template - Make cooldown cache max-size configurable via Spring property - Replace manual getters/setters with Lombok in NotificationExecutorProperties - Add @min validation constraints on executor pool properties - Fix atomic replace in NotificationCooldownCache (remove+put → put) - Add unit tests: EmailService, CooldownCache, NotificationService (33 tests) - Add Spring-context integration test for SMTP fallback pipeline - Add Testcontainers integration test for login with missing SMTP table Resolves: MX-241
DeathGun44
force-pushed
the
MX-241-harden-notification-smtp-fallback
branch
from
71b8ae8 to
bdf969d
Compare
IOhacker
approved these changes
Apr 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes