Merge branch 'master' into master

lemur/notifications/messaging.py

@@ -103,8 +103,9 @@ def send_plugin_notification(event_type, data, recipients, notification):
     function = f"{__name__}.{sys._getframe().f_code.co_name}"
     log_data = {
         "function": function,
-        "message": f"Sending expiration notification for to recipients {recipients}",
+        "message": f"Sending {event_type} notification for to recipients {recipients}",
-        "notification_type": "expiration",
+        "notification_type": event_type,
+        "notification_plugin": notification.plugin.slug,
         "certificate_targets": recipients,
     }
     status = FAILURE_METRIC_STATUS
@@ -121,7 +122,7 @@ def send_plugin_notification(event_type, data, recipients, notification):
         "notification",
         "counter",
         1,
-        metric_tags={"status": status, "event_type": event_type},
+        metric_tags={"status": status, "event_type": event_type, "plugin": notification.plugin.slug},
     )
 
     if status == SUCCESS_METRIC_STATUS:
@@ -142,7 +143,6 @@ def send_expiration_notifications(exclude):
 
         for notification_label, certificates in notification_group.items():
             notification_data = []
-            security_data = []
 
             notification = certificates[0][0]
 
@@ -152,33 +152,26 @@ def send_expiration_notifications(exclude):
                     certificate
                 ).data
                 notification_data.append(cert_data)
-                security_data.append(cert_data)
-
-            if send_default_notification(
-                    "expiration", notification_data, [owner], notification.options
-            ):
-                success += 1
-            else:
-                failure += 1
-
-            recipients = notification.plugin.filter_recipients(notification.options, security_email + [owner])
 
+            email_recipients = notification.plugin.get_recipients(notification.options, security_email + [owner])
+            # Plugin will ONLY use the provided recipients if it's email; any other notification plugin ignores them
             if send_plugin_notification(
-                "expiration",
+                    "expiration", notification_data, email_recipients, notification
-                notification_data,
-                recipients,
-                notification,
             ):
-                success += 1
+                success += len(email_recipients)
             else:
-                failure += 1
+                failure += len(email_recipients)
-
+            # If we're using an email plugin, we're done,
-            if send_default_notification(
+            #   since "security_email + [owner]" were added as email_recipients.
-                "expiration", security_data, security_email, notification.options
+            # If we're not using an email plugin, we also need to send an email to the security team and owner,
-            ):
+            #   since the plugin notification didn't send anything to them.
-                success += 1
+            if notification.plugin.slug != "email-notification":
-            else:
+                if send_default_notification(
-                failure += 1
+                        "expiration", notification_data, email_recipients, notification.options
+                ):
+                    success = 1 + len(email_recipients)
+                else:
+                    failure = 1 + len(email_recipients)
 
     return success, failure
 
@@ -195,15 +188,16 @@ def send_default_notification(notification_type, data, targets, notification_opt
     :return:
     """
     function = f"{__name__}.{sys._getframe().f_code.co_name}"
-    log_data = {
-        "function": function,
-        "message": f"Sending notification for certificate data {data}",
-        "notification_type": notification_type,
-    }
     status = FAILURE_METRIC_STATUS
     notification_plugin = plugins.get(
         current_app.config.get("LEMUR_DEFAULT_NOTIFICATION_PLUGIN", "email-notification")
     )
+    log_data = {
+        "function": function,
+        "message": f"Sending {notification_type} notification for certificate data {data} to targets {targets}",
+        "notification_type": notification_type,
+        "notification_plugin": notification_plugin.slug,
+    }
 
     try:
         current_app.logger.debug(log_data)
@@ -212,7 +206,7 @@ def send_default_notification(notification_type, data, targets, notification_opt
         status = SUCCESS_METRIC_STATUS
     except Exception as e:
         log_data["message"] = f"Unable to send {notification_type} notification for certificate data {data} " \
-                              f"to target {targets}"
+                              f"to targets {targets}"
         current_app.logger.error(log_data, exc_info=True)
         sentry.captureException()
 
@@ -220,7 +214,7 @@ def send_default_notification(notification_type, data, targets, notification_opt
         "notification",
         "counter",
         1,
-        metric_tags={"status": status, "event_type": notification_type},
+        metric_tags={"status": status, "event_type": notification_type, "plugin": notification_plugin.slug},
     )
 
     if status == SUCCESS_METRIC_STATUS:
@@ -247,15 +241,14 @@ def send_pending_failure_notification(
     data = pending_certificate_output_schema.dump(pending_cert).data
     data["security_email"] = current_app.config.get("LEMUR_SECURITY_TEAM_EMAIL")
 
-    notify_owner_success = False
+    email_recipients = []
     if notify_owner:
-        notify_owner_success = send_default_notification("failed", data, [data["owner"]], pending_cert)
+        email_recipients = email_recipients + [data["owner"]]
 
-    notify_security_success = False
     if notify_security:
-        notify_security_success = send_default_notification("failed", data, data["security_email"], pending_cert)
+        email_recipients = email_recipients + data["security_email"]
 
-    return notify_owner_success or notify_security_success
+    return send_default_notification("failed", data, email_recipients, pending_cert)
 
 
 def needs_notification(certificate):

lemur/plugins/bases/notification.py

@@ -20,14 +20,14 @@ class NotificationPlugin(Plugin):
     def send(self, notification_type, message, targets, options, **kwargs):
         raise NotImplementedError
 
-    def filter_recipients(self, options, excluded_recipients):
+    def get_recipients(self, options, additional_recipients):
         """
-        Given a set of options (which should include configured recipient info), filters out recipients that
+        Given a set of options (which should include configured recipient info), returns the parsed list of recipients
-        we do NOT want to notify.
+        from those options plus the additional recipients specified. The returned value has no duplicates.
 
-        For any notification types where recipients can't be dynamically modified, this returns an empty list.
+        For any notification types where recipients can't be dynamically modified, this returns only the additional recipients.
         """
-        return []
+        return additional_recipients
 
 
 class ExpirationNotificationPlugin(NotificationPlugin):

lemur/plugins/lemur_email/plugin.py

@@ -105,6 +105,8 @@ class EmailNotificationPlugin(ExpirationNotificationPlugin):
 
     @staticmethod
     def send(notification_type, message, targets, options, **kwargs):
+        if not targets:
+            return
 
         subject = "Lemur: {0} Notification".format(notification_type.capitalize())
 
@@ -119,11 +121,9 @@ class EmailNotificationPlugin(ExpirationNotificationPlugin):
             send_via_smtp(subject, body, targets)
 
     @staticmethod
-    def filter_recipients(options, excluded_recipients, **kwargs):
+    def get_recipients(options, additional_recipients, **kwargs):
         notification_recipients = get_plugin_option("recipients", options)
         if notification_recipients:
             notification_recipients = notification_recipients.split(",")
-            # removing owner and security_email from notification_recipient
-            notification_recipients = [i for i in notification_recipients if i not in excluded_recipients]
 
-        return notification_recipients
+        return list(set(notification_recipients + additional_recipients))

lemur/plugins/lemur_email/tests/test_email.py

@@ -21,7 +21,6 @@ def get_options():
 
 
 def test_render_expiration(certificate, endpoint):
-
     new_cert = CertificateFactory()
     new_cert.replaces.append(certificate)
 
@@ -54,7 +53,7 @@ def test_send_expiration_notification():
     certificate.notifications[0].options = get_options()
 
     verify_sender_email()
-    assert send_expiration_notifications([]) == (3, 0)  # owner, recipients (only counted as 1), and security
+    assert send_expiration_notifications([]) == (4, 0)  # owner (1), recipients (2), and security (1)
 
 
 @mock_ses
@@ -76,15 +75,20 @@ def test_send_pending_failure_notification(user, pending_certificate, async_issu
 
     verify_sender_email()
     assert send_pending_failure_notification(pending_certificate)
+    assert send_pending_failure_notification(pending_certificate, True, True)
+    assert send_pending_failure_notification(pending_certificate, True, False)
+    assert send_pending_failure_notification(pending_certificate, False, True)
+    assert send_pending_failure_notification(pending_certificate, False, False)
 
 
-def test_filter_recipients(certificate, endpoint):
+def test_get_recipients(certificate, endpoint):
     from lemur.plugins.lemur_email.plugin import EmailNotificationPlugin
 
-    options = [{"name": "recipients", "value": "security@example.com,bob@example.com,joe@example.com"}]
+    options = [{"name": "recipients", "value": "security@example.com,joe@example.com"}]
-    assert EmailNotificationPlugin.filter_recipients(options, []) == ["security@example.com", "bob@example.com",
+    two_emails = sorted(["security@example.com", "joe@example.com"])
-                                                                      "joe@example.com"]
+    assert sorted(EmailNotificationPlugin.get_recipients(options, [])) == two_emails
-    assert EmailNotificationPlugin.filter_recipients(options, ["security@example.com"]) == ["bob@example.com",
+    assert sorted(EmailNotificationPlugin.get_recipients(options, ["security@example.com"])) == two_emails
-                                                                                            "joe@example.com"]
+    three_emails = sorted(["security@example.com", "bob@example.com", "joe@example.com"])
-    assert EmailNotificationPlugin.filter_recipients(options, ["security@example.com", "bob@example.com",
+    assert sorted(EmailNotificationPlugin.get_recipients(options, ["bob@example.com"])) == three_emails
-                                                               "joe@example.com"]) == []
+    assert sorted(EmailNotificationPlugin.get_recipients(options, ["security@example.com", "bob@example.com",
+                                                                   "joe@example.com"])) == three_emails