Protect against unexpected needinfo field

Signed-off-by: Aurélien Bompard <[email protected]>

bugzilla2fedmsg/relay.py

@@ -8,7 +8,7 @@
 from fedora_messaging.exceptions import ConnectionException, PublishReturned
 from fedora_messaging.message import INFO
 
-from .utils import convert_datetimes, email_to_fas
+from .utils import convert_datetimes, email_to_fas, needinfo_email
 
 
 LOGGER = logging.getLogger(__name__)
@@ -172,9 +172,7 @@ def _get_all_emails(self, body):
                         emails.add(email)
             elif change["field"] == "flag.needinfo":
                 # anyone for whom a 'needinfo' flag is set
-                # this is extracting the email from a value like:
-                # "? ([email protected])"
-                email = change["added"].split("(", 1)[1].rsplit(")", 1)[0]
+                email = needinfo_email(change["added"])
                 if email:
                     emails.add(email)
 

bugzilla2fedmsg/utils.py

@@ -37,6 +37,16 @@ def convert_datetimes(obj):
             return obj
 
 
+def needinfo_email(flag_content):
+    # this is extracting the email from a value like:
+    # "? ([email protected])"
+    try:
+        return flag_content.split("(", 1)[1].rsplit(")", 1)[0]
+    except IndexError:
+        LOGGER.warning("Could not extract email from %r", flag_content)
+        return None
+
+
 def email_to_fas(email, fasjson):
     """Try to get a FAS username from an email address, return None if no FAS username is found"""
     if email.endswith("@fedoraproject.org"):

tests/test_relay.py

@@ -213,3 +213,31 @@ def test_publish_exception_connectionexception(testrelay, fakepublish, bug_creat
     assert fakepublish.call_count == 1
     # check the logging worked
     assert "Error sending message" in caplog.text
+
+
+def test_needinfo_removed(testrelay, fakepublish, bug_modify_message_four_changes):
+    bug_modify_message_four_changes["body"]["event"]["changes"][2] = {
+        "field": "cc",
+        "removed": "",
+        "added": "[email protected]",
+    }
+    bug_modify_message_four_changes["body"]["event"]["changes"][3]["added"] = ""
+    testrelay.on_stomp_message(
+        bug_modify_message_four_changes["body"], bug_modify_message_four_changes["headers"]
+    )
+    assert fakepublish.call_count == 1
+    message = fakepublish.call_args[0][0]
+    assert message.body["agent_name"] is None
+    assert message.body["usernames"] == ["adamw"]
+
+
+def test_needinfo_bad(testrelay, fakepublish, bug_modify_message_four_changes):
+    last_change = bug_modify_message_four_changes["body"]["event"]["changes"][3]
+    last_change["added"] = "some garbage"
+    try:
+        testrelay.on_stomp_message(
+            bug_modify_message_four_changes["body"], bug_modify_message_four_changes["headers"]
+        )
+    except IndexError as e:
+        pytest.fail(e)
+    assert fakepublish.call_count == 1

tests/test_schemas.py

@@ -37,27 +37,25 @@ def test_bug_create_schema(fakepublish, bug_create_message, testrelay):
     assert message.product_name == "Fedora"
     assert (
         message.summary
-        == "dgunchev@gmail.com filed a new bug RHBZ#1701391 'SELinux is preventing touch from 'write'...'"
+        == "dgunchev filed a new bug RHBZ#1701391 'SELinux is preventing touch from 'write'...'"
     )
     assert (
         str(message)
-        == "dgunchev@gmail.com filed a new bug RHBZ#1701391 'SELinux is preventing touch from 'write'...'"
+        == "dgunchev filed a new bug RHBZ#1701391 'SELinux is preventing touch from 'write'...'"
     )
     assert message.url == "https://bugzilla.redhat.com/show_bug.cgi?id=1701391"
     assert (
         message.app_icon == "https://bugzilla.redhat.com/extensions/RedHat/web/css/favicon.ico?v=0"
     )
-    assert message.agent_name == "Doncho Gunchev"
-    assert message.app_name == "bugzilla2fedmsg"
-    # broken till we can do email2fas
-    assert message.usernames == []
+    assert message.agent_name == "dgunchev"
+    assert message.app_name == "Bugzilla"
+    assert message.usernames == ["dgunchev", "lv"]
     assert message.packages == ["selinux-policy"]
     assert (
         message.agent_avatar
         == "https://seccdn.libravatar.org/avatar/d4bfc5ec5260361c930aad299c8e14fe03af45109ea88e880a191851b8c83e7f?s=64&d=retro"
     )
     assert message._primary_email == "[email protected]"
-    assert message._all_emails == ["[email protected]", "[email protected]"]
 
 
 def test_bug_modify_schema(fakepublish, bug_modify_message, testrelay):
@@ -71,16 +69,7 @@ def test_bug_modify_schema(fakepublish, bug_modify_message, testrelay):
         message.summary
         == "[email protected] updated 'cc' on RHBZ#1699203 'python-pyramid-1.10.4 is available'"
     )
-    # here we test both picking up an address from a 'cc' change
-    # event, and filtering out lists.fedoraproject.org addresses
-    assert message._all_emails == [
-        "[email protected]",
-        "[email protected]",
-        "[email protected]",
-    ]
-    # here we test that we can at least derive usernames from
-    # fedoraproject.org email addresses
-    assert message.usernames == ["upstream-release-monitoring"]
+    assert message.usernames == ["adamw", "upstream-release-monitoring"]
 
 
 def test_bug_modify_four_changes_schema(fakepublish, bug_modify_message_four_changes, testrelay):
@@ -99,12 +88,7 @@ def test_bug_modify_four_changes_schema(fakepublish, bug_modify_message_four_cha
         message.summary
         == "[email protected] updated 'assigned_to', 'bug_status', 'cc', and 'flag.needinfo' on RHBZ#1702701 'Review Request: perl-Class-AutoClass - D...'"
     )
-    # this tests gathering an address from a 'needinfo' change
-    assert message._all_emails == [
-        "[email protected]",
-        "[email protected]",
-        "[email protected]",
-    ]
+    assert message.usernames == []
 
 
 def test_bug_modify_two_changes_schema(fakepublish, bug_modify_message_four_changes, testrelay):
@@ -146,10 +130,7 @@ def test_bug_modify_no_changes_schema(fakepublish, bug_modify_message, testrelay
         message.summary
         == "[email protected] updated something unknown on RHBZ#1699203 'python-pyramid-1.10.4 is available'"
     )
-    assert message._all_emails == [
-        "[email protected]",
-        "[email protected]",
-    ]
+    assert message.usernames == ["upstream-release-monitoring"]
 
 
 def test_comment_create_schema(fakepublish, comment_create_message, testrelay):
@@ -176,7 +157,7 @@ def test_attachment_create_schema(fakepublish, attachment_create_message, testre
     message.validate()
     assert (
         message.summary
-        == "peter@sonniger-tag.eu added attachment on RHBZ#1701353 '[abrt] gnome-software: gtk_widget_unpare...'"
+        == "peter added attachment on RHBZ#1701353 '[abrt] gnome-software: gtk_widget_unpare...'"
     )
 
 
@@ -191,7 +172,7 @@ def test_attachment_modify_schema(fakepublish, attachment_modify_message, testre
     message.validate()
     assert (
         message.summary
-        == "[email protected] updated 'isobsolete' for attachment on RHBZ#1701766 'I2C_HID_QUIRK_NO_IRQ_AFTER_RESET caused ...'"
+        == "joe updated 'isobsolete' for attachment on RHBZ#1701766 'I2C_HID_QUIRK_NO_IRQ_AFTER_RESET caused ...'"
     )
 
 
@@ -213,7 +194,7 @@ def test_attachment_modify_no_changes_schema(fakepublish, attachment_modify_mess
     message.validate()
     assert (
         message.summary
-        == "[email protected] updated something unknown for attachment on RHBZ#1701766 'I2C_HID_QUIRK_NO_IRQ_AFTER_RESET caused ...'"
+        == "joe updated something unknown for attachment on RHBZ#1701766 'I2C_HID_QUIRK_NO_IRQ_AFTER_RESET caused ...'"
     )