From 1a65e09a9996b48471612383fa129c0279176ea9 Mon Sep 17 00:00:00 2001 From: Jasmine Schladen Date: Wed, 11 Nov 2020 15:21:40 -0800 Subject: [PATCH 1/4] Send a single email to multiple recipients instead of multiple emails --- lemur/notifications/messaging.py | 77 +++++++++---------- lemur/plugins/bases/notification.py | 10 +-- lemur/plugins/lemur_email/plugin.py | 8 +- lemur/plugins/lemur_email/tests/test_email.py | 23 ++++-- 4 files changed, 61 insertions(+), 57 deletions(-) diff --git a/lemur/notifications/messaging.py b/lemur/notifications/messaging.py index 75d227b1..4ba32b38 100644 --- a/lemur/notifications/messaging.py +++ b/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}", - "notification_type": "expiration", + "message": f"Sending {event_type} notification for to recipients {recipients}", + "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,30 @@ 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 + email_recipients = security_email + [owner] + if notification.plugin.slug == "email-notification": + email_recipients = notification.plugin.get_recipients(notification.options, email_recipients) + if send_plugin_notification( + "expiration", notification_data, email_recipients, notification + ): + success += len(email_recipients) + else: + failure += len(email_recipients) else: - failure += 1 + if send_default_notification( + "expiration", notification_data, email_recipients, notification.options + ): + success += len(email_recipients) + else: + failure += len(email_recipients) - recipients = notification.plugin.filter_recipients(notification.options, security_email + [owner]) - - if send_plugin_notification( - "expiration", - notification_data, - recipients, - notification, - ): - success += 1 - else: - failure += 1 - - if send_default_notification( - "expiration", security_data, security_email, notification.options - ): - success += 1 - else: - failure += 1 + if send_plugin_notification( + "expiration", notification_data, [], notification + ): + success += 1 + else: + failure += 1 return success, failure @@ -195,15 +192,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 +210,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 +218,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 +245,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): diff --git a/lemur/plugins/bases/notification.py b/lemur/plugins/bases/notification.py index 76aa33de..03de95ce 100644 --- a/lemur/plugins/bases/notification.py +++ b/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 - we do NOT want to notify. + Given a set of options (which should include configured recipient info), returns the parsed list of recipients + 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): diff --git a/lemur/plugins/lemur_email/plugin.py b/lemur/plugins/lemur_email/plugin.py index 041b27ec..214586ab 100644 --- a/lemur/plugins/lemur_email/plugin.py +++ b/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)) diff --git a/lemur/plugins/lemur_email/tests/test_email.py b/lemur/plugins/lemur_email/tests/test_email.py index fd4dc575..448c4f5c 100644 --- a/lemur/plugins/lemur_email/tests/test_email.py +++ b/lemur/plugins/lemur_email/tests/test_email.py @@ -54,7 +54,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 +76,22 @@ 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): from lemur.plugins.lemur_email.plugin import EmailNotificationPlugin - options = [{"name": "recipients", "value": "security@example.com,bob@example.com,joe@example.com"}] - assert EmailNotificationPlugin.filter_recipients(options, []) == ["security@example.com", "bob@example.com", - "joe@example.com"] - assert EmailNotificationPlugin.filter_recipients(options, ["security@example.com"]) == ["bob@example.com", - "joe@example.com"] - assert EmailNotificationPlugin.filter_recipients(options, ["security@example.com", "bob@example.com", - "joe@example.com"]) == [] + options = [{"name": "recipients", "value": "security@example.com,joe@example.com"}] + assert sorted(EmailNotificationPlugin.get_recipients(options, [])) == sorted([ + "security@example.com", "joe@example.com"]) + assert sorted(EmailNotificationPlugin.get_recipients(options, ["security@example.com"])) == sorted([ + "security@example.com", "joe@example.com"]) + assert sorted(EmailNotificationPlugin.get_recipients(options, ["bob@example.com"])) == sorted([ + "security@example.com", "bob@example.com", "joe@example.com"]) + assert sorted(EmailNotificationPlugin.get_recipients(options, + ["security@example.com", "bob@example.com", "joe@example.com"])) == sorted([ + "security@example.com", "bob@example.com", "joe@example.com"]) From fc7db4a9b20d81ce1b6bf5245b0d0b871e86f46b Mon Sep 17 00:00:00 2001 From: Jasmine Schladen Date: Fri, 13 Nov 2020 13:13:37 -0800 Subject: [PATCH 2/4] Fix style --- lemur/plugins/lemur_email/tests/test_email.py | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/lemur/plugins/lemur_email/tests/test_email.py b/lemur/plugins/lemur_email/tests/test_email.py index 448c4f5c..3522f21c 100644 --- a/lemur/plugins/lemur_email/tests/test_email.py +++ b/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) @@ -82,16 +81,14 @@ def test_send_pending_failure_notification(user, pending_certificate, async_issu 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,joe@example.com"}] - assert sorted(EmailNotificationPlugin.get_recipients(options, [])) == sorted([ - "security@example.com", "joe@example.com"]) - assert sorted(EmailNotificationPlugin.get_recipients(options, ["security@example.com"])) == sorted([ - "security@example.com", "joe@example.com"]) - assert sorted(EmailNotificationPlugin.get_recipients(options, ["bob@example.com"])) == sorted([ - "security@example.com", "bob@example.com", "joe@example.com"]) - assert sorted(EmailNotificationPlugin.get_recipients(options, - ["security@example.com", "bob@example.com", "joe@example.com"])) == sorted([ - "security@example.com", "bob@example.com", "joe@example.com"]) + two_emails = sorted(["security@example.com", "joe@example.com"]) + assert sorted(EmailNotificationPlugin.get_recipients(options, [])) == two_emails + assert sorted(EmailNotificationPlugin.get_recipients(options, ["security@example.com"])) == two_emails + three_emails = sorted(["security@example.com", "bob@example.com", "joe@example.com"]) + assert sorted(EmailNotificationPlugin.get_recipients(options, ["bob@example.com"])) == three_emails + assert sorted(EmailNotificationPlugin.get_recipients(options, ["security@example.com", "bob@example.com", + "joe@example.com"])) == three_emails From db11f0c1b7edf5cb7e901b0bea2328dd76e626ee Mon Sep 17 00:00:00 2001 From: Jasmine Schladen Date: Fri, 13 Nov 2020 20:10:21 -0800 Subject: [PATCH 3/4] Condense sending notifications --- lemur/notifications/messaging.py | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/lemur/notifications/messaging.py b/lemur/notifications/messaging.py index 4ba32b38..5f414b8c 100644 --- a/lemur/notifications/messaging.py +++ b/lemur/notifications/messaging.py @@ -153,29 +153,23 @@ def send_expiration_notifications(exclude): ).data notification_data.append(cert_data) - email_recipients = security_email + [owner] - if notification.plugin.slug == "email-notification": - email_recipients = notification.plugin.get_recipients(notification.options, email_recipients) - if send_plugin_notification( - "expiration", notification_data, email_recipients, notification - ): - success += len(email_recipients) - else: - failure += len(email_recipients) + 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", notification_data, email_recipients, notification + ): + success += len(email_recipients) else: + failure += len(email_recipients) + # If we're using an email plugin, we're done; + # if not, we also need to send an email notification to the security team and owner + if notification.plugin.slug != "email-notification": if send_default_notification( "expiration", notification_data, email_recipients, notification.options ): - success += len(email_recipients) + success = 1 + len(email_recipients) else: - failure += len(email_recipients) - - if send_plugin_notification( - "expiration", notification_data, [], notification - ): - success += 1 - else: - failure += 1 + failure = 1 + len(email_recipients) return success, failure From 9aaf507dd65083dd8e198470cefd9f4dac1704c5 Mon Sep 17 00:00:00 2001 From: Jasmine Schladen Date: Mon, 16 Nov 2020 17:36:59 -0800 Subject: [PATCH 4/4] Clarify comment --- lemur/notifications/messaging.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lemur/notifications/messaging.py b/lemur/notifications/messaging.py index 5f414b8c..2658e1a0 100644 --- a/lemur/notifications/messaging.py +++ b/lemur/notifications/messaging.py @@ -161,8 +161,10 @@ def send_expiration_notifications(exclude): success += len(email_recipients) else: failure += len(email_recipients) - # If we're using an email plugin, we're done; - # if not, we also need to send an email notification to the security team and owner + # If we're using an email plugin, we're done, + # since "security_email + [owner]" were added as email_recipients. + # 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. if notification.plugin.slug != "email-notification": if send_default_notification( "expiration", notification_data, email_recipients, notification.options