Feat/optimize webhook payload detection (#947)

* feat: implement select_best_model function

* feat: return model class and instance

* fix: update webhook handler to use consistent parameter naming

* feat: add payload webhooks validation function

* feat: setup dev script to add mock webhooks

* fix: handle models with all optional fields

* chore: fmt

* fix: update UpptimePayload to require 'text' field

* refactor: simplify validate_payload function

* refactor: update webhook handling to improve payload validation and error handling

* refactor: enhance webhook payload handling with type casting

* refactor: improve test structure and utilize test_client fixture for consistency

* fix: update tag to proper value Webhooks

* refactor: update webhook handling to return structured results

* chore: update docstring for clarity

* feat: add WebhookResult model

* feat: enhance payload validation with logging

* chore: lint

Author: gcharest

app/api/v1/routes/webhooks.py

@@ -1,147 +1,199 @@
-import requests  # type: ignore
 import json
-from fastapi import APIRouter, Request, HTTPException
-from models.webhooks import AwsSnsPayload, WebhookPayload
-from core.logging import get_module_logger
+from typing import Union, Dict, Any, cast
+
+import requests  # type: ignore
 from api.dependencies.rate_limits import get_limiter
+from core.logging import get_module_logger
+from fastapi import APIRouter, HTTPException, Request, Body
 from integrations.sentinel import log_to_sentinel
+from models.webhooks import (
+    AwsSnsPayload,
+    WebhookPayload,
+    AccessRequest,
+    UpptimePayload,
+    WebhookResult,
+)
 from modules.slack import webhooks
+from modules.webhooks.base import validate_payload
 from server.event_handlers import aws
-from server.utils import (
-    log_ops_message,
-)
+from server.utils import log_ops_message
 
 
 logger = get_module_logger()
 router = APIRouter(tags=["Access"])
 limiter = get_limiter()
 
 
-@router.post("/hook/{id}")
+@router.post("/hook/{webhook_id}")
 @limiter.limit(
     "30/minute"
 )  # since some slack channels use this for alerting, we want to be generous with the rate limiting on this one
 def handle_webhook(
-    id: str,
-    payload: WebhookPayload | str,
+    webhook_id: str,
     request: Request,
+    payload: Union[Dict[Any, Any], str] = Body(...),
 ):
-    webhook = webhooks.get_webhook(id)
-    webhook_payload = WebhookPayload()
-    if webhook:
-        hook_type: str = webhook.get("hook_type", {"S": "alert"})["S"]
-        # if the webhook is active, then send forward the response to the webhook
-        if webhooks.is_active(id):
-            webhooks.increment_invocation_count(id)
-            if isinstance(payload, str):
-                processed_payload = handle_string_payload(payload, request)
-                if isinstance(processed_payload, dict):
-                    return processed_payload
-                else:
-                    logger.info(
-                        "payload_processed",
-                        payload=processed_payload,
-                        webhook_id=id,
-                    )
-                    webhook_payload = processed_payload
-            else:
-                webhook_payload = payload
-            webhook_payload.channel = webhook["channel"]["S"]
-            if hook_type == "alert":
-                webhook_payload = append_incident_buttons(webhook_payload, id)
-            try:
-                webhook_payload_parsed = webhook_payload.model_dump(exclude_none=True)
-                request.state.bot.client.api_call(
-                    "chat.postMessage", json=webhook_payload_parsed
-                )
-                log_to_sentinel(
-                    "webhook_sent",
-                    {"webhook": webhook, "payload": webhook_payload_parsed},
-                )
-                return {"ok": True}
-            except Exception as e:
-                logger.exception(
-                    "webhook_posting_error",
-                    webhook_id=id,
-                    webhook_payload=webhook_payload,
-                    error=str(e),
-                )
-                body = webhook_payload.model_dump(exclude_none=True)
-                log_ops_message(
-                    request.state.bot.client, f"Error posting message: ```{body}```"
-                )
-                raise HTTPException(
-                    status_code=500, detail="Failed to send message"
-                ) from e
-        else:
-            logger.info(
-                "webhook_not_active",
-                webhook_id=id,
-                webhook_payload=webhook_payload,
-                error="Webhook is not active",
-            )
-            raise HTTPException(status_code=404, detail="Webhook not active")
+    """Handle incoming webhook requests and post to Slack channel.
+
+    Args:
+        webhook_id (str): The ID of the webhook to handle.
+        request (Request): The incoming HTTP request.
+        payload (Union[Dict[Any, Any], str]): The incoming webhook payload, either as
+            a JSON string or a dictionary.
+
+    Raises:
+        HTTPException: If the webhook is not found, not active, or if there are issues
+            with payload validation or posting to Slack.
+    Returns:
+        dict: A dictionary indicating success if the message was posted successfully.
+    """
+    if isinstance(payload, dict):
+        payload_dict = payload
     else:
+        try:
+            payload_dict = json.loads(payload)
+        except json.JSONDecodeError as e:
+            logger.error("payload_validation_error", error=str(e), payload=str(payload))
+            raise HTTPException(status_code=400, detail=str(e)) from e
+
+    webhook = webhooks.get_webhook(webhook_id)
+    if not webhook:
         raise HTTPException(status_code=404, detail="Webhook not found")
 
+    if not webhook.get("active", {}).get("BOOL", False):
+        logger.info(
+            "webhook_not_active",
+            webhook_id=webhook_id,
+            error="Webhook is not active",
+        )
+        raise HTTPException(status_code=404, detail="Webhook not active")
+    webhooks.increment_invocation_count(webhook_id)
 
-def handle_string_payload(
-    payload: str,
+    webhook_result = handle_webhook_payload(payload_dict, request)
+
+    if webhook_result.status == "error":
+        raise HTTPException(status_code=400, detail="Invalid payload")
+
+    if webhook_result.action == "post" and isinstance(
+        webhook_result.payload, WebhookPayload
+    ):
+        webhook_payload = webhook_result.payload
+        webhook_payload.channel = webhook["channel"]["S"]
+        hook_type = webhook["hook_type"]["S"]
+        if hook_type == "alert":
+            webhook_payload = append_incident_buttons(webhook_payload, webhook_id)
+
+        webhook_payload_parsed = webhook_payload.model_dump(exclude_none=True)
+
+        try:
+            request.state.bot.client.api_call(
+                "chat.postMessage", json=webhook_payload_parsed
+            )
+            log_to_sentinel(
+                "webhook_sent",
+                {"webhook": webhook, "payload": webhook_payload_parsed},
+            )
+
+        except Exception as e:
+            logger.exception(
+                "webhook_posting_error",
+                webhook_id=webhook_id,
+                error=str(e),
+            )
+            raise HTTPException(status_code=500, detail="Failed to send message") from e
+
+    return {"ok": True}
+
+
+def handle_webhook_payload(
+    payload_dict: dict,
     request: Request,
-) -> WebhookPayload | dict:
+) -> WebhookResult:
+    """Process and validate the webhook payload.
 
-    string_payload_type, validated_payload = webhooks.validate_string_payload_type(
-        payload
-    )
-    logger.info(
-        "string_payload_type",
-        payload=payload,
-        string_payload_type=string_payload_type,
-        validated_payload=validated_payload,
+    Returns:
+        dict: A dictionary containing:
+            - status (str): The status of the operation (e.g., "success", "error").
+            - action (Literal["post", "log", "none"]): The action to take.
+            - payload (Optional[WebhookPayload]): The payload to post, if applicable.
+    """
+    logger.info("processing_webhook_payload", payload=payload_dict)
+    payload_validation_result = validate_payload(payload_dict)
+
+    webhook_result = WebhookResult(
+        status="error", message="Failed to process payload for unknown reasons"
     )
-    match string_payload_type:
+    if payload_validation_result is not None:
+        payload_type, validated_payload = payload_validation_result
+    else:
+        error_message = "No matching model found for payload"
+        return WebhookResult(status="error", message=error_message)
+
+    match payload_type.__name__:
         case "WebhookPayload":
-            webhook_payload = WebhookPayload(**validated_payload)
+            webhook_result = WebhookResult(
+                status="success", action="post", payload=validated_payload
+            )
         case "AwsSnsPayload":
-            awsSnsPayload = aws.validate_sns_payload(
-                AwsSnsPayload(**validated_payload),
+            aws_sns_payload_instance = cast(AwsSnsPayload, validated_payload)
+            aws_sns_payload = aws.validate_sns_payload(
+                aws_sns_payload_instance,
                 request.state.bot.client,
             )
-            if awsSnsPayload.Type == "SubscriptionConfirmation":
-                requests.get(awsSnsPayload.SubscribeURL, timeout=60)
+
+            if aws_sns_payload.Type == "SubscriptionConfirmation":
+                requests.get(aws_sns_payload.SubscribeURL, timeout=60)
                 logger.info(
                     "subscribed_webhook_to_topic",
-                    webhook_id=awsSnsPayload.TopicArn,
-                    subscribed_topic=awsSnsPayload.TopicArn,
+                    webhook_id=aws_sns_payload.TopicArn,
+                    subscribed_topic=aws_sns_payload.TopicArn,
                 )
                 log_ops_message(
                     request.state.bot.client,
-                    f"Subscribed webhook {id} to topic {awsSnsPayload.TopicArn}",
+                    f"Subscribed webhook {id} to topic {aws_sns_payload.TopicArn}",
+                )
+                webhook_result = WebhookResult(
+                    status="success", action="log", payload=None
                 )
-                return {"ok": True}
-            if awsSnsPayload.Type == "UnsubscribeConfirmation":
+
+            if aws_sns_payload.Type == "UnsubscribeConfirmation":
                 log_ops_message(
                     request.state.bot.client,
-                    f"{awsSnsPayload.TopicArn} unsubscribed from webhook {id}",
+                    f"{aws_sns_payload.TopicArn} unsubscribed from webhook {id}",
+                )
+                webhook_result = WebhookResult(
+                    status="success", action="log", payload=None
                 )
-                return {"ok": True}
-            if awsSnsPayload.Type == "Notification":
-                blocks = aws.parse(awsSnsPayload, request.state.bot.client)
-                # if we have an empty message, log that we have an empty
-                # message and return without posting to slack
+
+            if aws_sns_payload.Type == "Notification":
+                blocks = aws.parse(aws_sns_payload, request.state.bot.client)
                 if not blocks:
                     logger.info(
                         "payload_empty_message",
+                        payload_type="AwsSnsPayload",
+                        sns_type=aws_sns_payload.Type,
                     )
-                    return {"ok": True}
-                webhook_payload = WebhookPayload(blocks=blocks)
+                    return WebhookResult(
+                        status="error",
+                        action="none",
+                        message="Empty AWS SNS Notification message",
+                    )
+                webhook_result = WebhookResult(
+                    status="success",
+                    action="post",
+                    payload=WebhookPayload(blocks=blocks),
+                )
+
         case "AccessRequest":
-            # Temporary fix for the Access Request payloads
-            message = json.dumps(validated_payload)
-            webhook_payload = WebhookPayload(text=message)
+            message = str(cast(AccessRequest, validated_payload).model_dump())
+            webhook_result = WebhookResult(
+                status="success",
+                action="post",
+                payload=WebhookPayload(text=message),
+            )
+
         case "UpptimePayload":
-            # Temporary fix for Upptime payloads
-            text = validated_payload.get("text", "")
+            text = cast(UpptimePayload, validated_payload).text
             header_text = "📈 Web Application Status Changed!"
             blocks = [
                 {"type": "section", "text": {"type": "mrkdwn", "text": " "}},
@@ -157,16 +209,22 @@ def handle_string_payload(
                     },
                 },
             ]
-            webhook_payload = WebhookPayload(blocks=blocks)
+            webhook_result = WebhookResult(
+                status="success",
+                action="post",
+                payload=WebhookPayload(blocks=blocks),
+            )
+
         case _:
-            raise HTTPException(
-                status_code=500,
-                detail="Invalid payload type. Must be a WebhookPayload object or a recognized string payload type.",
+            webhook_result = WebhookResult(
+                status="error",
+                message="No matching model found for payload",
             )
-    return WebhookPayload(**webhook_payload.model_dump(exclude_none=True))
+
+    return webhook_result
 
 
-def append_incident_buttons(payload: WebhookPayload, webhook_id):
+def append_incident_buttons(payload: WebhookPayload, webhook_id) -> WebhookPayload:
     if payload.attachments is None:
         payload.attachments = []
     elif isinstance(payload.attachments, str):

app/bin/add_dev_webhooks.sh

@@ -0,0 +1,36 @@
+#!/bin/bash
+
+add_webhook() {
+  local TABLE_NAME=$1
+  local CHANNEL=$2
+  local USER_ID=$3
+  local NAME=$4
+  local HOOK_TYPE=${5:-"alert"}
+  local CREATED_AT=$(date -u +"%Y-%m-%dT%H:%M:%SZ")
+  local ID=$(date +%s%N | sha256sum | head -c 32) # Generate a unique ID
+
+  aws dynamodb put-item \
+    --table-name $TABLE_NAME \
+    --item "{\"id\": {\"S\": \"$ID\"}, \"channel\": {\"S\": \"$CHANNEL\"}, \"name\": {\"S\": \"$NAME\"}, \"created_at\": {\"S\": \"$CREATED_AT\"}, \"active\": {\"BOOL\": true}, \"user_id\": {\"S\": \"$USER_ID\"}, \"invocation_count\": {\"N\": \"0\"}, \"acknowledged_count\": {\"N\": \"0\"}, \"hook_type\": {\"S\": \"$HOOK_TYPE\"}}" \
+    --endpoint-url http://dynamodb-local:8000 \
+    --no-cli-pager
+
+  if [ $? -eq 0 ]; then
+    echo "Webhook added with ID: $ID"
+  else
+    echo "Failed to add webhook"
+  fi
+}
+
+if [ "$#" -lt 4 ]; then
+  echo "Usage: $0 <table_name> <channel> <user_id> <name> [hook_type]"
+  exit 1
+fi
+
+TABLE_NAME=$1
+CHANNEL=$2
+USER_ID=$3
+NAME=$4
+HOOK_TYPE=$5
+
+add_webhook $TABLE_NAME $CHANNEL $USER_ID $NAME $HOOK_TYPE