Merge pull request #2445 from nordic-institute/XRDDEV-2773

fix: Broken notifications breaks Certificate status change flow

src/common/common-mail/src/main/java/org/niis/xroad/common/mail/MailService.java

@@ -28,13 +28,17 @@
 import ee.ria.xroad.common.SystemProperties;
 
 import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
 import org.apache.commons.lang3.StringUtils;
 import org.springframework.mail.SimpleMailMessage;
 import org.springframework.mail.javamail.JavaMailSender;
 import org.springframework.stereotype.Service;
 
 import java.util.List;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
 
+@Slf4j
 @Service
 @RequiredArgsConstructor
 public class MailService {
@@ -57,7 +61,32 @@ public MailNotificationStatus getMailNotificationStatus() {
         return new MailNotificationStatus(successStatus, failureStatus, configurationPresent, recipientsEmails);
     }
 
-    public void sendMail(String to, String subject, String text) {
+    /** Sends mail notification without stopping the flow in case of error */
+    public void sendMailAsync(String to, String subject, String text) {
+        if (!mailNotificationProperties.isMailNotificationConfigurationPresent()) {
+            log.error("Attempted to send mail notification, but configuration is incomplete. Message wasn't sent!");
+            return;
+        }
+        SimpleMailMessage message = new SimpleMailMessage();
+        message.setTo(to);
+        message.setSubject(subject);
+        message.setText(text);
+        try (ExecutorService executorService = Executors.newSingleThreadExecutor()) {
+            executorService.execute(() -> sendMail(message));
+        }
+    }
+
+    private void sendMail(SimpleMailMessage message) {
+        try {
+            mailSender.send(message);
+        } catch (Exception e) {
+            log.error("Failed to send mail notification", e);
+        }
+    }
+
+    /** NB! Meant only for test notifications!
+     * For regular notification sending use {@link #sendMailAsync(String, String, String)} which is non-blocking */
+    public void sendTestMail(String to, String subject, String text) {
         SimpleMailMessage message = new SimpleMailMessage();
         message.setTo(to);
         message.setSubject(subject);

src/common/common-mail/src/test/java/org/niis/xroad/common/mail/MailServiceTest.java

@@ -30,13 +30,20 @@
 import org.junit.runner.RunWith;
 import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnitRunner;
+import org.springframework.mail.MailSendException;
+import org.springframework.mail.SimpleMailMessage;
 import org.springframework.mail.javamail.JavaMailSender;
 
 import java.util.List;
 import java.util.Map;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.verify;
 
 @RunWith(MockitoJUnitRunner.class)
 public class MailServiceTest {
@@ -66,6 +73,18 @@ public void getMailNotificationStatus() {
         assertEquals(List.of("TestMember: myMail@example.org"), mailNotificationStatus.recipientsEmails());
     }
 
+    @Test
+    public void sendTestMailThrows() {
+        doThrow(new MailSendException("Fatal error!")).when(mailSender).send((SimpleMailMessage) any());
+        assertThrows(MailSendException.class, () -> mailService.sendTestMail("recipient", "subject", "body"));
+        verify(mailSender).send((SimpleMailMessage) any());
+    }
 
+    @Test
+    public void sendMailAsyncDoesntThrows() {
+        doThrow(new MailSendException("Fatal error!")).when(mailSender).send((SimpleMailMessage) any());
+        assertDoesNotThrow(() -> mailService.sendMailAsync("recipient", "subject", "body"));
+        verify(mailSender).send((SimpleMailMessage) any());
+    }
 
 }

src/security-server/admin-service/application/src/main/java/org/niis/xroad/securityserver/restapi/scheduling/GlobalConfChecker.java

@@ -302,8 +302,8 @@ private void updateCertStatus(SecurityServerId securityServerId, CertificateInfo
                     // do nothing
                 }
                 case CertificateInfo.STATUS_REGINPROG -> {
-                    mailNotificationHelper.sendAuthCertRegisteredNotification(securityServerId, certInfo);
                     setCertStatus(cert, CertificateInfo.STATUS_REGISTERED, certInfo);
+                    mailNotificationHelper.sendAuthCertRegisteredNotification(securityServerId, certInfo);
                 }
                 case CertificateInfo.STATUS_SAVED, CertificateInfo.STATUS_GLOBALERR ->
                         setCertStatus(cert, CertificateInfo.STATUS_REGISTERED, certInfo);

src/security-server/admin-service/application/src/main/java/org/niis/xroad/securityserver/restapi/util/MailNotificationHelper.java

@@ -69,7 +69,7 @@ public void sendSuccessNotification(ClientId memberId,
             String content = KeyUsageInfo.AUTHENTICATION.equals(keyUsageInfo) ? authCertContent : signCertContent;
             Optional.ofNullable(mailNotificationProperties.getContacts())
                     .map(contacts -> contacts.get(memberId.asEncodedId()))
-                    .ifPresent(address -> mailService.sendMail(address, title, content));
+                    .ifPresent(address -> mailService.sendMailAsync(address, title, content));
         }
     }
 
@@ -95,7 +95,7 @@ public void sendFailureNotification(String memberId,
             String content = isSignCert ? signCertContent : authCertContent;
             Optional.ofNullable(mailNotificationProperties.getContacts())
                     .map(contacts -> contacts.get(memberId))
-                    .ifPresent(address -> mailService.sendMail(address, title, content));
+                    .ifPresent(address -> mailService.sendMailAsync(address, title, content));
         }
     }
 
@@ -108,12 +108,12 @@ public void sendAuthCertRegisteredNotification(SecurityServerId securityServerId
                             new String[]{certInfo.getCertificateDisplayName(), securityServerId.getServerCode()});
             Optional.ofNullable(mailNotificationProperties.getContacts())
                     .map(contacts -> contacts.get(securityServerId.getOwner().asEncodedId()))
-                    .ifPresent(address -> mailService.sendMail(address, title, content));
+                    .ifPresent(address -> mailService.sendMailAsync(address, title, content));
         }
     }
 
     public void sendTestMail(String recipientAddress, String securityServerId) {
-        mailService.sendMail(recipientAddress,
+        mailService.sendTestMail(recipientAddress,
                 notificationMessageSourceAccessor.getMessage("test_mail_title", new String[]{securityServerId}),
                 notificationMessageSourceAccessor.getMessage("test_mail_content", new String[]{securityServerId}));
     }

src/security-server/admin-service/application/src/test/java/org/niis/xroad/securityserver/restapi/openapi/MailApiControllerTest.java

@@ -96,7 +96,7 @@ public void testSendMail() {
     @WithMockUser(authorities = {"DIAGNOSTICS"})
     public void testSendMailException() {
 
-        doThrow(new MailSendException("Sending failed")).when(mailService).sendMail(any(), any(), any());
+        doThrow(new MailSendException("Sending failed")).when(mailService).sendTestMail(any(), any(), any());
         ResponseEntity<TestMailResponse> testMailResponse =
                 mailApiController.sendTestMail(new MailRecipient("test@mailaddress.org"));
         assertEquals(MailStatus.ERROR, testMailResponse.getBody().getStatus());