#959 copr-fe-dev shouldn't send emails to everybody
Merged a year ago by praiskup. Opened a year ago by schlupov.
copr/ schlupov/copr sending_emails  into  master

@@ -74,7 +74,7 @@ 

  class Notifier(object):

      def notify(self, user, chroots):

          msg = OutdatedChrootMessage(chroots)

-         send_mail(user.mail, msg)

+         send_mail([user.mail], msg)

Personally, I would preserve the same input types for send_mail function (i.e. both one email or list of emails) and therefore omitting this and many similar changes like this one that are lower in this PR.

It was added in the same commit as the whitelisting feature, but it is not necessary for that feature, so I am interested in the idea behind it :-)

Aha, that was probably related to this, right?

nitpick, don't you want to s/recepient/recepients/? And fix the comment to "List of recepient emails" or something?

@praiskup do you prefer it this way?

I believed that we removed this multiple-types-for-one-arguments from send_mail, if we did not - we should IMO

Ok, no problem with me. Let's accept just list of email addresses. I was just a little bit confused because it was in the same commit as the feature and they are not related to each other.

  

      def store_timestamp(self, chroots):

          for chroot in chroots:

@@ -93,6 +93,8 @@ 

      ENABLE_DISCUSSION = False

      DISCOURSE_URL = ''

  

+     WHITELIST_EMAILS = []

+ 

  class ProductionConfig(Config):

      DEBUG = False

      # SECRET_KEY = "put_some_secret_here"

@@ -1,6 +1,6 @@ 

  import flask

  import platform

- import smtplib

+ from smtplib import SMTP

  from email.mime.text import MIMEText

  from coprs import app, helpers

  

@@ -114,9 +114,21 @@ 

                  "{3}\n\n".format(chroot.copr.full_name, chroot.name, chroot.delete_after_days, url))

  

  

- def send_mail(recipient, message, sender=None, reply_to=None):

+ def filter_whitelisted_recipients(recipients):

      """

-     :param str/list recipient: One recipient email as a string or multiple emails in a list

+     Filters e-mail recipients if the white list of recipients in conf is not empty.

+     :param recipients: list of recipients

+     :return: list of recipients who should receive an e-mail

+     """

+     if not app.config["WHITELIST_EMAILS"]:

+         return recipients

+ 

+     return [r for r in recipients if r in app.config["WHITELIST_EMAILS"]]

+ 

+ 

+ def send_mail(recipients, message, sender=None, reply_to=None):

+     """

+     :param list recipients: List of email recipients

      :param Message message:

      :param str sender: Email of a sender

      :return:

@@ -124,8 +136,10 @@ 

      msg = MIMEText(message.text)

      msg["Subject"] = message.subject

      msg["From"] = sender or "root@{0}".format(platform.node())

-     msg["To"] = ", ".join(recipient) if type(recipient) == list else recipient

+     msg["To"] = ", ".join(recipients)

      msg.add_header("reply-to", reply_to or app.config["REPLY_TO"])

-     s = smtplib.SMTP("localhost")

-     s.sendmail("root@{0}".format(platform.node()), recipient, msg.as_string())

-     s.quit()

+     recipients = filter_whitelisted_recipients(recipients)

+     if not recipients:

+         return

+     with SMTP("localhost") as smtp:

+         smtp.sendmail("root@{0}".format(platform.node()), recipients, msg.as_string())

@@ -67,7 +67,7 @@ 

      # send emails only if transaction succeeded

      for task in messages:

          if flask.current_app.config.get("SEND_EMAILS", False):

-             send_mail(task['address'], task['message'])

+             send_mail([task['address']], task['message'])

  

      return flask.jsonify({'updated': list(updated.keys())})

  

@@ -96,6 +96,6 @@ 

          msg = PermissionRequestMessage(copr, flask.g.user, permission_dict)

          for address in copr.admin_mails:

              if flask.current_app.config.get("SEND_EMAILS", False):

-                 send_mail(address, msg)

+                 send_mail([address], msg)

  

      return flask.jsonify({'updated': bool(permission_dict)})

@@ -522,7 +522,7 @@ 

                  permission_dict = {"old_builder": old_builder, "old_admin": old_admin,

                                     "new_builder": new_builder, "new_admin": new_admin}

                  msg = PermissionRequestMessage(copr, flask.g.user, permission_dict)

-                 send_mail(mail, msg, )

+                 send_mail([mail], msg)

  

      return flask.redirect(helpers.copr_url("coprs_ns.copr_detail", copr))

  

@@ -213,8 +213,8 @@ 

          emails = ['user1@spam.foo', 'user2@spam.foo']

          calls = send_mail.call_args_list

          assert len(calls) == 2

-         assert calls[0][0][0] == "user2@spam.foo"

-         assert calls[1][0][0] == "user1@foo.bar"

+         assert calls[0][0][0] == ["user2@spam.foo"]

+         assert calls[1][0][0] == ["user1@foo.bar"]

          assert str(calls[0][0][1]) == msg

          assert str(calls[1][0][1]) == msg

  

@@ -1,6 +1,6 @@ 

  import pytest

  import datetime

- from coprs.mail import PermissionRequestMessage, PermissionChangeMessage, LegalFlagMessage, OutdatedChrootMessage

+ from coprs.mail import PermissionRequestMessage, PermissionChangeMessage, LegalFlagMessage, OutdatedChrootMessage, filter_whitelisted_recipients

  from tests.coprs_test_case import CoprsTestCase

  from coprs import app

  

@@ -84,3 +84,16 @@ 

          with pytest.raises(AttributeError) as ex:

              OutdatedChrootMessage(copr_chroots=[])

          assert "No outdated chroots" in str(ex)

+ 

+     def test_filter_recipients(self):

+         app.config["WHITELIST_EMAILS"] = ["test@redhat.com"]

+         recipient = filter_whitelisted_recipients(["test@redhat.com", "user@redhat.com"])

+         assert recipient == ["test@redhat.com"]

+ 

+         app.config["WHITELIST_EMAILS"] = ["test@redhat.com", "user@redhat.com"]

+         recipient = filter_whitelisted_recipients(["test@redhat.com", "user@redhat.com"])

+         assert recipient == ["test@redhat.com", "user@redhat.com"]

+ 

+         app.config["WHITELIST_EMAILS"] = []

+         recipient = filter_whitelisted_recipients(["test@redhat.com", "user@redhat.com"])

+         assert recipient == ["test@redhat.com", "user@redhat.com"]

rebased onto de97ef30539cc73d64d99d80c48b3dbcab4a3351

a year ago

why this if condition? Just use send_mail([user.mail]), unless you really expect that user.mail variable can hold array value.

rebased onto 9d771f85357e1a801702fbeff9f6e8e43144a7e5

a year ago

1 new commit added

  • Adding test for function filter_recipients
a year ago

nitpick, don't you want to s/recepient/recepients/? And fix the comment to "List of recepient emails" or something?

I'd simply do:

if not app.config["WHITELIST_EMAILS"]: 
    return recepients

Because if the config option is empty (production), it is useless to iterate over all the recipients.

and you can do return [r for r in recepients if r in app.config["WHITELIST_EMAILS"]]

rebased onto 95def9224952b2e2cdafa03ca3085fbf2026e9cf

a year ago

Personally, I would preserve the same input types for send_mail function (i.e. both one email or list of emails) and therefore omitting this and many similar changes like this one that are lower in this PR.

It was added in the same commit as the whitelisting feature, but it is not necessary for that feature, so I am interested in the idea behind it :-)

Aha, that was probably related to this, right?

nitpick, don't you want to s/recepient/recepients/? And fix the comment to "List of recepient emails" or something?

@praiskup do you prefer it this way?

Yeah, I sort of disliked doing if type(recepient) on multiple places. No huge preference, it was rather suggestion since I think that stronger typing is usually better.

nitpick: I would prefer a slightly longer, but more descriptive function name, e.g. filter_whitelisted_recipients. This is IMHO too generic and when reading its name, I have no idea why/what would I even filter.

I believed that we removed this multiple-types-for-one-arguments from send_mail, if we did not - we should IMO

nitpick: I am absolutely aware of how unimportant this is for the majority of people, and how annoying I am for always pointing that out, but can you please move the condition above the with statement, reverse it to if not ... and return if it is met? This will avoid unnecessary nesting and make me happy. Also, it would be a good idea to log some message, that the email will not be sent because there is no whitelisted recipient.

Ok, no problem with me. Let's accept just list of email addresses. I was just a little bit confused because it was in the same commit as the feature and they are not related to each other.

Ok, I think we should have these two commits squashed.

rebased onto 78be9b966b6513c9eb33a4ed7d1a109838e7bd0f

a year ago

rebased onto 05e170c8399b4ecfe13250686bd2221dd95f88bf

a year ago

rebased onto 81f2925

a year ago

Commit 51e452a fixes this pull-request

Pull-Request has been merged by praiskup

a year ago