From 3ff58428b20a037480b4dc5fd605c92a1675f02b Mon Sep 17 00:00:00 2001
From: Pierre-Yves Chibon <pingou@pingoured.fr>
Date: Jul 29 2019 12:36:02 +0000
Subject: [PATCH 1/2] Ensure @<username> doesn't over-reach when sending notifications


If someone comments in a ticket or a PR with a text that contains an
email address, for example: foo@bar.com and the domain corresponds to an
existing username, we do not want to notify that user.
(Imagine if an `gmail` user gets created! :D)

Signed-off-by: Pierre-Yves Chibon <pingou@pingoured.fr>

---

diff --git a/pagure/lib/notify.py b/pagure/lib/notify.py
index 77f7c64..7670ad1 100644
--- a/pagure/lib/notify.py
+++ b/pagure/lib/notify.py
@@ -34,6 +34,7 @@ import flask
 import pagure.lib.query
 import pagure.lib.tasks_services
 from pagure.config import config as pagure_config
+from pagure.pfmarkdown import MENTION_RE
 
 
 _log = logging.getLogger(__name__)
@@ -233,8 +234,7 @@ def _add_mentioned_users(emails, comment):
     """ Check the comment to see if an user is mentioned in it and if
     so add this user to the list of people to notify.
     """
-    mentio_re = r"@(\w+)"
-    for username in re.findall(mentio_re, comment):
+    for username in re.findall(MENTION_RE, comment):
         user = pagure.lib.query.search_user(flask.g.session, username=username)
         if user:
             emails.add(user.default_email)
diff --git a/pagure/pfmarkdown.py b/pagure/pfmarkdown.py
index b46f48f..47d49a5 100644
--- a/pagure/pfmarkdown.py
+++ b/pagure/pfmarkdown.py
@@ -52,7 +52,7 @@ except ImportError:
 # MENTION_RE regex). Note that it is a zero-length match - it does
 # not capture or consume any of the string - and it does not appear
 # as a group for the match object.
-MENTION_RE = r"(?<!\w)@(\w+)"
+MENTION_RE = r"(?<![\w\-\"\'\`\$\!\*\+#%&/=^{}|~])@(\w+)"
 # Each line below correspond to a line of the regex:
 #  1) Don't start matching in the middle of a word
 #  2) See if there is a `forks/` at the start
diff --git a/tests/test_pagure_lib_notify_email.py b/tests/test_pagure_lib_notify_email.py
index 9678a7b..b0d9ea4 100644
--- a/tests/test_pagure_lib_notify_email.py
+++ b/tests/test_pagure_lib_notify_email.py
@@ -15,6 +15,7 @@ import sys
 import os
 
 import mock
+import munch
 import six
 
 sys.path.insert(
@@ -141,6 +142,120 @@ http://localhost.localdomain/test/issue/1
         self.assertEqual(kwargs["user_from"], self.user1.fullname)
 
     @mock.patch("pagure.lib.notify.send_email")
+    def test_user_notified_new_comment(self, fakemail):
+        """Check that users that are @mentionned are notified."""
+        self.comment1.comment = "Hey @foo. Let's do it!"
+        g = munch.Munch()
+        g.session = self.session
+        with mock.patch("flask.g", g):
+            pagure.lib.notify.notify_new_comment(self.comment1)
+
+        (_, args, kwargs) = fakemail.mock_calls[0]
+
+        # Mail should be sent to both users
+        self.assertIn(
+            args[2],
+            ["bar@pingou.com,foo@bar.com", "foo@bar.com,bar@pingou.com"],
+        )
+
+        # Mail ID should be comment #1's mail ID...
+        self.assertEqual(kwargs["mail_id"], self.comment1.mail_id)
+
+        # In reply to issue #1's mail ID.
+        self.assertEqual(kwargs["in_reply_to"], self.issue1.mail_id)
+
+        # Project name should be...project (full) name.
+        self.assertEqual(kwargs["project_name"], self.project1.fullname)
+
+        # Mail should be from user1 (who wrote the comment).
+        self.assertEqual(kwargs["user_from"], self.user1.fullname)
+
+    @mock.patch("pagure.lib.notify.send_email")
+    def test_user_notified_new_comment_start_row(self, fakemail):
+        """Check that users that are @mentionned are notified."""
+        self.comment1.comment = "@foo, okidoki"
+        g = munch.Munch()
+        g.session = self.session
+        with mock.patch("flask.g", g):
+            pagure.lib.notify.notify_new_comment(self.comment1)
+
+        (_, args, kwargs) = fakemail.mock_calls[0]
+
+        # Mail should be sent to both users
+        self.assertIn(
+            args[2],
+            ["bar@pingou.com,foo@bar.com", "foo@bar.com,bar@pingou.com"],
+        )
+
+        # Mail ID should be comment #1's mail ID...
+        self.assertEqual(kwargs["mail_id"], self.comment1.mail_id)
+
+        # In reply to issue #1's mail ID.
+        self.assertEqual(kwargs["in_reply_to"], self.issue1.mail_id)
+
+        # Project name should be...project (full) name.
+        self.assertEqual(kwargs["project_name"], self.project1.fullname)
+
+        # Mail should be from user1 (who wrote the comment).
+        self.assertEqual(kwargs["user_from"], self.user1.fullname)
+
+    @mock.patch("pagure.lib.notify.send_email")
+    def test_user_notified_new_comment_with_email(self, fakemail):
+        """Ensures that @mention doesn't over-reach."""
+        self.comment1.comment = "So apparently bar@foo.com exists"
+        g = munch.Munch()
+        g.fas_user = tests.FakeUser(username="pingou")
+        g.authenticated = True
+        g.session = self.session
+        with mock.patch("flask.g", g):
+            pagure.lib.notify.notify_new_comment(self.comment1)
+
+        (_, args, kwargs) = fakemail.mock_calls[0]
+
+        # Mail should be sent to both users
+        self.assertEqual(args[2], "bar@pingou.com")
+
+        # Mail ID should be comment #1's mail ID...
+        self.assertEqual(kwargs["mail_id"], self.comment1.mail_id)
+
+        # In reply to issue #1's mail ID.
+        self.assertEqual(kwargs["in_reply_to"], self.issue1.mail_id)
+
+        # Project name should be...project (full) name.
+        self.assertEqual(kwargs["project_name"], self.project1.fullname)
+
+        # Mail should be from user1 (who wrote the comment).
+        self.assertEqual(kwargs["user_from"], self.user1.fullname)
+
+    @mock.patch("pagure.lib.notify.send_email")
+    def test_user_notified_new_comment_with_email_with_number(self, fakemail):
+        """Ensures that @mention doesn't over-reach."""
+        self.comment1.comment = "So apparently bar123@foo.com exists"
+        g = munch.Munch()
+        g.fas_user = tests.FakeUser(username="pingou")
+        g.authenticated = True
+        g.session = self.session
+        with mock.patch("flask.g", g):
+            pagure.lib.notify.notify_new_comment(self.comment1)
+
+        (_, args, kwargs) = fakemail.mock_calls[0]
+
+        # Mail should be sent to both users
+        self.assertEqual(args[2], "bar@pingou.com")
+
+        # Mail ID should be comment #1's mail ID...
+        self.assertEqual(kwargs["mail_id"], self.comment1.mail_id)
+
+        # In reply to issue #1's mail ID.
+        self.assertEqual(kwargs["in_reply_to"], self.issue1.mail_id)
+
+        # Project name should be...project (full) name.
+        self.assertEqual(kwargs["project_name"], self.project1.fullname)
+
+        # Mail should be from user1 (who wrote the comment).
+        self.assertEqual(kwargs["user_from"], self.user1.fullname)
+
+    @mock.patch("pagure.lib.notify.send_email")
     def test_notify_new_issue_namespaced(
         self, fakemail
     ):  # pylint: disable=invalid-name

From d437dbcbdfb4feaec4e827179f523b63dfac2e55 Mon Sep 17 00:00:00 2001
From: Pierre-Yves Chibon <pingou@pingoured.fr>
Date: Jul 29 2019 12:48:08 +0000
Subject: [PATCH 2/2] Fix tests for new arrow version


Signed-off-by: Pierre-Yves Chibon <pingou@pingoured.fr>

---

diff --git a/tests/test_pagure_admin.py b/tests/test_pagure_admin.py
index ef9aef4..bbc8999 100644
--- a/tests/test_pagure_admin.py
+++ b/tests/test_pagure_admin.py
@@ -1633,7 +1633,7 @@ class PagureBlockUserTests(tests.Modeltests):
             pagure.cli.admin.do_block_user(args)
 
         output = output.getvalue()
-        self.assertEqual("No users are currently blocked\n", output)
+        self.assertIn("No users are currently blocked\n", output)
 
     @patch("pagure.cli.admin._ask_confirmation", MagicMock(return_value=True))
     def test_list_blocked_user_with_date_and_data(self):
@@ -1653,7 +1653,7 @@ class PagureBlockUserTests(tests.Modeltests):
             pagure.cli.admin.do_block_user(args)
 
         output = output.getvalue()
-        self.assertEqual(
+        self.assertIn(
             "Users blocked:\n"
             " pingou                -  2050-12-31T00:00:00\n",
             output,
@@ -1667,7 +1667,7 @@ class PagureBlockUserTests(tests.Modeltests):
             pagure.cli.admin.do_block_user(args)
 
         output = output.getvalue()
-        self.assertEqual("No users are currently blocked\n", output)
+        self.assertIn("No users are currently blocked\n", output)
 
 
 class PagureAdminDeleteProjectTests(tests.Modeltests):