return 200 for an unhandled webhook decison, with the reason (#21411)

* return 200 for an unhandled webhook decison, with the reason

* move decision action processing into the view

* Update views.py

src/olympia/abuse/models.py

@@ -476,11 +476,11 @@ class CinderReport(ModelBase):
         )
         self.update(job_id=job_id)
 
-    def process_decision(self, *, decision_id, decision_date, decision_actions):
+    def process_decision(self, *, decision_id, decision_date, decision_action):
         self.update(
             decision_id=decision_id,
             decision_date=decision_date,
-            **({'decision_action': decision_actions[0]} if decision_actions else {}),
+            decision_action=decision_action,
         )
         self.get_action_helper().process()
 

src/olympia/abuse/tests/assets/cinder_webhook.json

@@ -4,16 +4,14 @@
         "appeal": {
             "appealed_decision": {
                 "enforcement_actions": [
-                    "delete-status",
-                    "freeze-account"
+                    "amo_disable_addon"
                 ],
                 "id": "ae106e36-700c-4668-9618-851451dc5c2c",
                 "notes": "",
                 "policies": [
                     {
                         "enforcement_actions": [
-                            "delete-status",
-                            "freeze-account"
+                            "amo_disable_addon"
                         ],
                         "id": "f73ad527-54ed-430c-86ff-80e15e2a352b",
                         "is_illegal": false,
@@ -36,8 +34,7 @@
             }
         },
         "enforcement_actions": [
-            "delete-status",
-            "freeze-account"
+            "amo_disable_addon"
         ],
         "entity": {
             "attributes": {
@@ -52,8 +49,7 @@
         "policies": [
             {
                 "enforcement_actions": [
-                    "delete-status",
-                    "freeze-account"
+                    "amo_disable_addon"
                 ],
                 "id": "f73ad527-54ed-430c-86ff-80e15e2a352b",
                 "is_illegal": false,

src/olympia/abuse/tests/test_models.py

@@ -451,7 +451,7 @@ class TestCinderReport(TestCase):
             cinder_report.process_decision(
                 decision_id='12345',
                 decision_date=new_date,
-                decision_actions=[CinderReport.DECISION_ACTIONS.AMO_APPROVE],
+                decision_action=CinderReport.DECISION_ACTIONS.AMO_APPROVE.value,
             )
         assert cinder_report.decision_id == '12345'
         assert cinder_report.decision_date == new_date

src/olympia/abuse/tests/test_views.py

@@ -2,7 +2,7 @@ import hashlib
 import hmac
 import json
 import os
-from datetime import datetime, timezone
+from datetime import datetime
 from unittest import mock
 
 from django.test.utils import override_settings
@@ -819,15 +819,15 @@ class TestCinderWebhook(TestCase):
         self._setup_report()
         req = self.get_request()
         with mock.patch.object(CinderReport, 'process_decision') as process_mock:
-            cinder_webhook(req)
+            response = cinder_webhook(req)
             process_mock.assert_called()
             process_mock.assert_called_with(
                 decision_id='d1f01fae-3bce-41d5-af8a-e0b4b5ceaaed',
-                decision_date=datetime(
-                    2023, 10, 12, 9, 8, 37, 4789, tzinfo=timezone.utc
-                ),
-                decision_actions=['delete-status', 'freeze-account'],
+                decision_date=datetime(2023, 10, 12, 9, 8, 37, 4789),
+                decision_action=CinderReport.DECISION_ACTIONS.AMO_DISABLE_ADDON.value,
             )
+        assert response.status_code == 201
+        assert response.data == {'amo': {'received': True, 'handled': True}}
 
     def test_wrong_queue(self):
         self._setup_report()
@@ -835,8 +835,16 @@ class TestCinderWebhook(TestCase):
         data['payload']['source']['job']['queue']['slug'] = 'another-queue'
         req = self.get_request(data=data)
         with mock.patch.object(CinderReport, 'process_decision') as process_mock:
-            cinder_webhook(req)
+            response = cinder_webhook(req)
             process_mock.assert_not_called()
+        assert response.status_code == 200
+        assert response.data == {
+            'amo': {
+                'received': True,
+                'handled': False,
+                'not_handled_reason': 'Not from a queue we process',
+            }
+        }
 
     def test_not_decision_event(self):
         self._setup_report()
@@ -844,14 +852,45 @@ class TestCinderWebhook(TestCase):
         data['event'] = 'report.created'
         req = self.get_request(data=data)
         with mock.patch.object(CinderReport, 'process_decision') as process_mock:
-            cinder_webhook(req)
+            response = cinder_webhook(req)
             process_mock.assert_not_called()
+        assert response.status_code == 200
+        assert response.data == {
+            'amo': {
+                'received': True,
+                'handled': False,
+                'not_handled_reason': 'Not a decision',
+            }
+        }
 
     def test_no_cinder_report(self):
         req = self.get_request()
         with mock.patch.object(CinderReport, 'process_decision') as process_mock:
-            cinder_webhook(req)
+            response = cinder_webhook(req)
             process_mock.assert_not_called()
+        assert response.status_code == 200
+        assert response.data == {
+            'amo': {
+                'received': True,
+                'handled': False,
+                'not_handled_reason': 'No matching job id found',
+            }
+        }
+
+    def test_invalid_decision_action(self):
+        self._setup_report()
+        data = self.get_data()
+        data['payload']['enforcement_actions'] = []
+        req = self.get_request(data=data)
+        response = cinder_webhook(req)
+        assert response.status_code == 200
+        assert response.data == {
+            'amo': {
+                'received': True,
+                'handled': False,
+                'not_handled_reason': 'Payload invalid: "decision_actions" malformed',
+            }
+        }
 
 
 class RatingAbuseViewSetTestBase:

src/olympia/abuse/views.py

@@ -1,10 +1,10 @@
 import hashlib
 import hmac
-from datetime import datetime
+from datetime import datetime, timezone
 
 from django import forms
 from django.conf import settings
-from django.core.exceptions import PermissionDenied
+from django.core.exceptions import PermissionDenied, ValidationError
 from django.http import Http404
 from django.shortcuts import get_object_or_404
 from django.template.response import TemplateResponse
@@ -157,7 +157,11 @@ class CinderInboundPermission:
 
 def process_datestamp(date_string):
     try:
-        return datetime.fromisoformat(date_string.replace(' ', ''))
+        return (
+            datetime.fromisoformat(date_string.replace(' ', ''))
+            .astimezone(timezone.utc)
+            .replace(tzinfo=None)
+        )
     except ValueError:
         log.warn('Invalid timestamp from cinder webhook %s', date_string)
         return datetime.now()
@@ -167,34 +171,64 @@ def process_datestamp(date_string):
 @authentication_classes(())
 @permission_classes((CinderInboundPermission,))
 def cinder_webhook(request):
-    if request.data.get('event') == 'decision.created' and (
-        payload := request.data.get('payload', {})
-    ):
+    try:
+        if request.data.get('event') != 'decision.created':
+            log.info('Non-decision payload received: %s', str(request.data)[:255])
+            raise ValidationError('Not a decision')
+
+        if not (payload := request.data.get('payload', {})):
+            log.info('No payload received: %s', str(request.data)[:255])
+            raise ValidationError('No payload')
+
         source = payload.get('source', {})
         job = source.get('job', {})
-        if (queue_name := job.get('queue', {}).get('slug')) == CinderEntity.queue:
-            log.info('Valid Payload from AMO queue: %s', payload)
-            job_id = job.get('id', '')
-            decision_id = source.get('decision', {}).get('id')
-            actions = payload.get('enforcement_actions')
-            try:
-                cinder_report = CinderReport.objects.get(job_id=job_id)
-                cinder_report.process_decision(
-                    decision_id=decision_id,
-                    decision_date=process_datestamp(payload.get('timestamp')),
-                    decision_actions=actions,
-                )
-            except CinderReport.DoesNotExist:
-                log.debug('CinderReport instance not found for job id %s', job_id)
-        else:
+        if (queue_name := job.get('queue', {}).get('slug')) != CinderEntity.queue:
             log.info('Payload from other queue: %s', queue_name)
-    else:
-        log.info(
-            'Non-decision payload received: %s',
-            str(request.data)[:255],
+            raise ValidationError('Not from a queue we process')
+
+        if (
+            not (actions := payload.get('enforcement_actions'))
+            or len(actions) > 1
+            or not CinderReport.DECISION_ACTIONS.has_api_value(actions[0])
+        ):
+            log.exception(
+                'Cinder webhook request failed: bad enforcement_actions [%s]', actions
+            )
+            raise ValidationError('Payload invalid: "decision_actions" malformed')
+
+        log.info('Valid Payload from AMO queue: %s', payload)
+        job_id = job.get('id', '')
+        try:
+            cinder_report = CinderReport.objects.get(job_id=job_id)
+        except CinderReport.DoesNotExist:
+            log.debug('CinderReport instance not found for job id %s', job_id)
+            raise ValidationError('No matching job id found')
+
+        cinder_report.process_decision(
+            decision_id=source.get('decision', {}).get('id'),
+            decision_date=process_datestamp(payload.get('timestamp')),
+            decision_action=CinderReport.DECISION_ACTIONS.for_api_value(
+                actions[0]
+            ).value,
         )
 
-    return Response(data={'amo-received': True}, status=status.HTTP_201_CREATED)
+    except ValidationError as exc:
+        return Response(
+            data={
+                'amo': {
+                    'received': True,
+                    'handled': False,
+                    'not_handled_reason': exc.message,
+                }
+            },
+            # cinder will retry indefinately on 4xx or 5xx so we return 200 even when
+            # it's an error
+            status=status.HTTP_200_OK,
+        )
+    return Response(
+        data={'amo': {'received': True, 'handled': True}},
+        status=status.HTTP_201_CREATED,
+    )
 
 
 def appeal(request, *, decision_id, **kwargs):

src/olympia/api/utils.py

@@ -34,6 +34,12 @@ class APIChoices(Choices):
     def api_choices(self):
         return tuple((entry[1], entry[0].lower()) for entry in self.entries)
 
+    def has_api_value(self, value):
+        return self.has_constant(value.upper() if value else value)
+
+    def for_api_value(self, value):
+        return self.for_constant(value.upper() if value else value)
+
 
 class APIChoicesWithNone(APIChoices):
     """Like APIChoices, but also returns 'None' as a valid choice for `choices`