Fix Jira integration error handling and type representation (#14320)

* fix: update to_str_typed function to use __name__ for type representation

* fix: handle errors when pushing findings to Jira in FindingSerializer

* refactor: update push_to_jira and related functions to return status and messages

* fix: prevent request failure by skipping JIRA push when not configured in test case

Author: Maffooch

dojo/api_v2/serializers.py

@@ -1890,7 +1890,9 @@ def update(self, instance, validated_data):
 
         if push_to_jira or finding_helper.is_keep_in_sync_with_jira(instance):
             # Push synchronously so that we can see jira errors in real time
-            jira_helper.push_to_jira(instance, sync=True)
+            success, message = jira_helper.push_to_jira(instance, sync=True)
+            if not success:
+                raise serializers.ValidationError(message)
 
         return instance
 

dojo/jira_link/helper.py

@@ -757,7 +757,7 @@ def jira_environment(obj):
     return ""
 
 
-def push_to_jira(obj, *args, **kwargs):
+def push_to_jira(obj, *args, **kwargs) -> tuple[str, bool]:
     if obj is None:
         msg = "Cannot push None to JIRA"
         raise ValueError(msg)
@@ -773,29 +773,32 @@ def push_to_jira(obj, *args, **kwargs):
 
     if isinstance(obj, Engagement):
         return dojo_dispatch_task(push_engagement_to_jira, obj.id, *args, **kwargs)
-    logger.error("unsupported object passed to push_to_jira: %s %i %s", obj.__name__, obj.id, obj)
-    return None
+    message = f"unsupported object passed to push_to_jira: {obj.__class__.__name__} {obj.id} {obj}"
+    logger.error(message)
+    return False, message
 
 
 # we need thre separate celery tasks due to the decorators we're using to map to/from ids
 @app.task
-def push_finding_to_jira(finding_id, *args, **kwargs):
+def push_finding_to_jira(finding_id, *args, **kwargs) -> tuple[str, bool]:
     finding = get_object_or_none(Finding, id=finding_id)
     if not finding:
-        logger.warning("Finding with id %s does not exist, skipping push_finding_to_jira", finding_id)
-        return None
+        message = f"Finding with id {finding_id} does not exist, skipping push_finding_to_jira"
+        logger.warning(message)
+        return False, message
 
     if finding.has_jira_issue:
         return update_jira_issue(finding, *args, **kwargs)
     return add_jira_issue(finding, *args, **kwargs)
 
 
 @app.task
-def push_finding_group_to_jira(finding_group_id, *args, **kwargs):
+def push_finding_group_to_jira(finding_group_id, *args, **kwargs) -> tuple[str, bool]:
     finding_group = get_object_or_none(Finding_Group, id=finding_group_id)
     if not finding_group:
-        logger.warning("Finding_Group with id %s does not exist, skipping push_finding_group_to_jira", finding_group_id)
-        return None
+        message = f"Finding_Group with id {finding_group_id} does not exist, skipping push_finding_group_to_jira"
+        logger.warning(message)
+        return False, message
 
     # Look for findings that have single ticket associations separate from the group
     for finding in finding_group.findings.filter(jira_issue__isnull=False):
@@ -807,11 +810,12 @@ def push_finding_group_to_jira(finding_group_id, *args, **kwargs):
 
 
 @app.task
-def push_engagement_to_jira(engagement_id, *args, **kwargs):
+def push_engagement_to_jira(engagement_id, *args, **kwargs) -> tuple[str, bool]:
     engagement = get_object_or_none(Engagement, id=engagement_id)
     if not engagement:
-        logger.warning("Engagement with id %s does not exist, skipping push_engagement_to_jira", engagement_id)
-        return None
+        message = f"Engagement with id {engagement_id} does not exist, skipping push_engagement_to_jira"
+        logger.warning(message)
+        return False, message
 
     if engagement.has_jira_issue:
         return dojo_dispatch_task(update_epic, engagement.id, *args, **kwargs)
@@ -894,18 +898,18 @@ def prepare_jira_issue_fields(
     return fields
 
 
-def add_jira_issue(obj, *args, **kwargs):
-    def failure_to_add_message(message: str, exception: Exception, _: Any) -> bool:
+def add_jira_issue(obj, *args, **kwargs) -> tuple[str, bool]:
+    def failure_to_add_message(message: str, exception: Exception, _: Any) -> tuple[str, bool]:
         if exception:
             logger.error("Exception occurred", exc_info=exception)
         logger.error(message)
         log_jira_alert(message, obj)
-        return False
+        return False, message
 
     logger.info("trying to create a new jira issue for %d:%s", obj.id, to_str_typed(obj))
 
     if not is_jira_enabled():
-        return False
+        return False, "JIRA integration is not enabled."
 
     if not is_jira_configured_and_enabled(obj):
         message = f"Object {obj.id} cannot be pushed to JIRA as there is no JIRA configuration for {to_str_typed(obj)}."
@@ -932,20 +936,20 @@ def failure_to_add_message(message: str, exception: Exception, _: Any) -> bool:
 
         # not sure why this check is not part of can_be_pushed_to_jira, but afraid to change it
         if isinstance(obj, Finding) and obj.duplicate and not obj.active:
-            logger.info("%s will not be pushed to JIRA as it's a duplicate finding", to_str_typed(obj))
+            message = f"{to_str_typed(obj)} is a duplicate and inactive finding, and will not be pushed to JIRA: {error_message}."
             # Duplicates are expected, don't create alerts
-            logger.info("%s cannot be pushed to JIRA: %s (expected - duplicate finding)",
-                       to_str_typed(obj), error_message)
+            logger.info(message)
         elif error_code in expected_validation_errors:
+            message = f"{to_str_typed(obj)} cannot be pushed to JIRA: {error_message}."
             # These are expected when auto-pushing, only log, don't alert
-            logger.info("%s cannot be pushed to JIRA: %s (expected - finding not ready yet)",
-                       to_str_typed(obj), error_message)
+            logger.info(message)
         else:
             # Unexpected errors (configuration issues, etc.) should still alert
-            log_jira_cannot_be_pushed_reason(error_message, obj)
-            logger.warning("%s cannot be pushed to JIRA: %s.", to_str_typed(obj), error_message)
+            message = f"{to_str_typed(obj)} cannot be pushed to JIRA due to an unexpected error: {error_message}."
+            log_jira_cannot_be_pushed_reason(message, obj)
+            logger.warning("%s cannot be pushed to JIRA: %s.", to_str_typed(obj), message)
             logger.warning("The JIRA issue will NOT be created.")
-        return False
+        return False, message
     logger.debug("Trying to create a new JIRA issue for %s...", to_str_typed(obj))
     # Attempt to get the jira connection
     try:
@@ -1056,21 +1060,21 @@ def failure_to_add_message(message: str, exception: Exception, _: Any) -> bool:
         message = f"Failed to assign jira issue to existing epic: {e}"
         return failure_to_add_message(message, e, obj)
 
-    return True
+    return True, "JIRA issue created successfully."
 
 
-def update_jira_issue(obj, *args, **kwargs):
-    def failure_to_update_message(message: str, exception: Exception, obj: Any) -> bool:
+def update_jira_issue(obj, *args, **kwargs) -> tuple[str, bool]:
+    def failure_to_update_message(message: str, exception: Exception, obj: Any) -> tuple[str, bool]:
         if exception:
             logger.error(exception)
         logger.error(message)
         log_jira_alert(message, obj)
-        return False
+        return False, message
 
     logger.debug("trying to update a linked jira issue for %d:%s", obj.id, to_str_typed(obj))
 
     if not is_jira_enabled():
-        return False
+        return False, "JIRA integration is not enabled."
 
     jira_project = get_jira_project(obj)
     jira_instance = get_jira_instance(obj)
@@ -1181,7 +1185,7 @@ def failure_to_update_message(message: str, exception: Exception, obj: Any) -> b
         message = f"Failed to assign jira issue to existing epic: {e}"
         return failure_to_update_message(message, e, obj)
 
-    return True
+    return True, "JIRA issue updated successfully."
 
 
 def get_jira_issue_from_jira(find):
@@ -1447,13 +1451,14 @@ def close_epic(engagement_id, push_to_jira, **kwargs):
 def update_epic(engagement_id, **kwargs):
     engagement = get_object_or_none(Engagement, id=engagement_id)
     if not engagement:
-        logger.warning("Engagement with id %s does not exist, skipping update_epic", engagement_id)
-        return False
+        message = f"Engagement with id {engagement_id} does not exist, skipping update_epic"
+        logger.warning(message)
+        return False, message
 
     logger.debug("trying to update jira EPIC for %d:%s", engagement.id, engagement.name)
 
     if not is_jira_configured_and_enabled(engagement):
-        return False
+        return False, "JIRA integration is not properly configured for this engagement."
 
     logger.debug("config found")
 
@@ -1479,35 +1484,40 @@ def update_epic(engagement_id, **kwargs):
                 jira_issue_update_kwargs["priority"] = {"name": epic_priority}
             issue.update(**jira_issue_update_kwargs)
         except JIRAError as e:
-            logger.exception("Jira Engagement/Epic Update Error")
-            log_jira_generic_alert("Jira Engagement/Epic Update Error", str(e))
-            return False
+            message = f"Failed to update the JIRA EPIC for engagement {engagement.id} - {e}"
+            logger.exception(message)
+            log_jira_generic_alert(message)
+            return False, message
 
-        return True
+        return True, "JIRA EPIC updated successfully."
 
-    add_error_message_to_response("Push to JIRA for Epic skipped because enable_engagement_epic_mapping is not checked for this engagement")
-    return False
+    message = f"Push to JIRA for Epic skipped because enable_engagement_epic_mapping is not checked for engagement {engagement.id}."
+    add_error_message_to_response(message)
+    return False, message
 
 
 @app.task
 def add_epic(engagement_id, **kwargs):
     engagement = get_object_or_none(Engagement, id=engagement_id)
     if not engagement:
-        logger.warning("Engagement with id %s does not exist, skipping add_epic", engagement_id)
-        return False
+        message = f"Engagement with id {engagement_id} does not exist, skipping add_epic"
+        logger.warning(message)
+        return False, message
 
     logger.debug("trying to create a new jira EPIC for %d:%s", engagement.id, engagement.name)
 
     if not is_jira_configured_and_enabled(engagement):
-        return False
+        message = f"JIRA integration is not properly configured for engagement {engagement.id}."
+        return False, message
 
     logger.debug("config found")
 
     jira_project = get_jira_project(engagement)
     jira_instance = get_jira_instance(engagement)
     if not jira_instance:
-        logger.warning("JIRA add epic failed: jira_instance is None")
-        return False
+        message = f"JIRA add epic failed for engagement {engagement.id}: jira_instance is None"
+        logger.warning(message)
+        return False, message
 
     if jira_project and jira_project.enable_engagement_epic_mapping:
         epic_name = kwargs.get("epic_name")
@@ -1555,15 +1565,15 @@ def add_epic(engagement_id, **kwargs):
                 message = "The 'Epic name id' in your DefectDojo Jira Configuration does not appear to be correct. Please visit, " + jira_instance.url + \
                     "/rest/api/2/field and search for Epic Name. Copy the number out of cf[number] and place in your DefectDojo settings for Jira and try again. For example, if your results are cf[100001] then copy 100001 and place it in 'Epic name id'. (Your Epic Id will be different.) \n\n"
             logger.exception(message)
+            message = f"JIRA add epic failed for engagement {engagement.id}: {message}"
+            log_jira_generic_alert(message)
+            return False, message
 
-            log_jira_generic_alert("Jira Engagement/Epic Creation Error",
-                                   message + error)
-            return False
-
-        return True
+        return True, "JIRA EPIC created successfully."
 
-    add_error_message_to_response("Push to JIRA for Epic skipped because enable_engagement_epic_mapping is not checked for this engagement")
-    return False
+    message = f"Push to JIRA for Epic skipped because enable_engagement_epic_mapping is not checked for engagement {engagement.id}."
+    add_error_message_to_response(message)
+    return False, message
 
 
 def jira_get_issue(jira_project, issue_key):
@@ -1847,7 +1857,8 @@ def process_jira_epic_form(request, engagement=None):
                 epic_priority = None
                 if jira_epic_form.cleaned_data.get("epic_priority"):
                     epic_priority = jira_epic_form.cleaned_data.get("epic_priority")
-                if push_to_jira(engagement, epic_name=epic_name, epic_priority=epic_priority):
+                success, message = push_to_jira(engagement, epic_name=epic_name, epic_priority=epic_priority)
+                if success:
                     logger.debug("Push to JIRA for Epic queued successfully")
                     messages.add_message(
                         request,
@@ -1856,11 +1867,11 @@ def process_jira_epic_form(request, engagement=None):
                         extra_tags="alert-success")
                 else:
                     error = True
-                    logger.debug("Push to JIRA for Epic failey")
+                    logger.debug("Push to JIRA for Epic failed")
                     messages.add_message(
                         request,
                         messages.ERROR,
-                        "Push to JIRA for Epic failed, check alerts on the top right for errors",
+                        message,
                         extra_tags="alert-danger")
         else:
             logger.debug("invalid jira epic form")

dojo/utils.py

@@ -1971,7 +1971,7 @@ def mass_model_updater(model_type, models, function, fields, page_size=1000, ord
 
 def to_str_typed(obj):
     """For code that handles multiple types of objects, print not only __str__ but prefix the type of the object"""
-    return f"{type(obj)}: {obj}"
+    return f"{type(obj).__name__}: {obj}"
 
 
 def get_product(obj):

unittests/test_rest_framework.py

@@ -1788,7 +1788,8 @@ def __init__(self, *args, **kwargs):
             "files": [],
             "tags": ["tag1", "tag_2"],
         }
-        self.update_fields = {"duplicate": False, "active": True, "push_to_jira": "True", "tags": ["finding_tag_new"]}
+        # Do not push to jira here as it will make the request fail due to jira not being configured
+        self.update_fields = {"duplicate": False, "active": True, "tags": ["finding_tag_new"]}
         self.test_type = TestType.OBJECT_PERMISSIONS
         self.permission_check_class = Finding
         self.permission_create = Permissions.Finding_Add