diff --git a/kobo/apps/hook/models/hook_log.py b/kobo/apps/hook/models/hook_log.py index 2cba30815f..00ecd429e0 100644 --- a/kobo/apps/hook/models/hook_log.py +++ b/kobo/apps/hook/models/hook_log.py @@ -45,15 +45,15 @@ def can_retry(self) -> bool: Notice: even if False is returned, `self.retry()` can be triggered. """ if self.hook.active: - if self.tries >= constance.config.HOOK_MAX_RETRIES: - # If log is still pending after `constance.config.HOOK_MAX_RETRIES` - # times, there was an issue, we allow the retry. - threshold = timezone.now() - timedelta(seconds=120) - - return self.status == HOOK_LOG_FAILED or ( - self.date_modified < threshold - and self.status == HOOK_LOG_PENDING - ) + # If log is still pending after `constance.config.HOOK_MAX_RETRIES` + # times, there was an issue, we allow the retry. + threshold = timezone.now() - timedelta(seconds=120) + + return self.status == HOOK_LOG_FAILED or ( + self.date_modified < threshold + and self.status == HOOK_LOG_PENDING + and self.tries >= constance.config.HOOK_MAX_RETRIES + ) return False diff --git a/kobo/apps/hook/tests/hook_test_case.py b/kobo/apps/hook/tests/hook_test_case.py index b4ea20658e..c5c31d420a 100644 --- a/kobo/apps/hook/tests/hook_test_case.py +++ b/kobo/apps/hook/tests/hook_test_case.py @@ -1,6 +1,7 @@ # coding: utf-8 import json +import pytest import responses from django.conf import settings from django.urls import reverse @@ -11,6 +12,7 @@ from kpi.exceptions import BadFormatException from kpi.tests.kpi_test_case import KpiTestCase from ..constants import HOOK_LOG_FAILED +from ..exceptions import HookRemoteServerDownError from ..models import HookLog, Hook @@ -94,26 +96,45 @@ def _send_and_fail(self): :return: dict """ + first_hooklog_response = self._send_and_wait_for_retry() + + # Fakes Celery n retries by forcing status to `failed` + # (where n is `settings.HOOKLOG_MAX_RETRIES`) + first_hooklog = HookLog.objects.get( + uid=first_hooklog_response.get('uid') + ) + first_hooklog.change_status(HOOK_LOG_FAILED) + + return first_hooklog_response + + def _send_and_wait_for_retry(self): self.hook = self._create_hook() ServiceDefinition = self.hook.get_service_definition() submissions = self.asset.deployment.get_submissions(self.asset.owner) submission_id = submissions[0]['_id'] service_definition = ServiceDefinition(self.hook, submission_id) - first_mock_response = {'error': 'not found'} + first_mock_response = {'error': 'gateway timeout'} # Mock first request's try - responses.add(responses.POST, self.hook.endpoint, - json=first_mock_response, status=status.HTTP_404_NOT_FOUND) + responses.add( + responses.POST, + self.hook.endpoint, + json=first_mock_response, + status=status.HTTP_504_GATEWAY_TIMEOUT, + ) # Mock next requests' tries - responses.add(responses.POST, self.hook.endpoint, - status=status.HTTP_200_OK, - content_type='application/json') + responses.add( + responses.POST, + self.hook.endpoint, + status=status.HTTP_200_OK, + content_type='application/json', + ) # Try to send data to external endpoint - success = service_definition.send() - self.assertFalse(success) + with pytest.raises(HookRemoteServerDownError): + service_definition.send() # Retrieve the corresponding log url = reverse('hook-log-list', kwargs={ @@ -126,20 +147,13 @@ def _send_and_fail(self): # Result should match first try self.assertEqual( - first_hooklog_response.get('status_code'), status.HTTP_404_NOT_FOUND + first_hooklog_response.get('status_code'), + status.HTTP_504_GATEWAY_TIMEOUT, ) self.assertEqual( json.loads(first_hooklog_response.get('message')), first_mock_response, ) - - # Fakes Celery n retries by forcing status to `failed` - # (where n is `settings.HOOKLOG_MAX_RETRIES`) - first_hooklog = HookLog.objects.get( - uid=first_hooklog_response.get('uid') - ) - first_hooklog.change_status(HOOK_LOG_FAILED) - return first_hooklog_response def __prepare_submission(self): diff --git a/kobo/apps/hook/tests/test_api_hook.py b/kobo/apps/hook/tests/test_api_hook.py index b8fbe5f7f8..b5a62c1623 100644 --- a/kobo/apps/hook/tests/test_api_hook.py +++ b/kobo/apps/hook/tests/test_api_hook.py @@ -1,6 +1,7 @@ # coding: utf-8 import json +import pytest import responses from constance.test import override_config from django.urls import reverse @@ -22,6 +23,7 @@ ) from kpi.utils.datetime import several_minutes_from_now from .hook_test_case import HookTestCase, MockSSRFProtect +from ..exceptions import HookRemoteServerDownError class ApiHookTestCase(HookTestCase): @@ -169,18 +171,20 @@ def test_partial_update_hook(self): self.assertFalse(hook.active) self.assertEqual(hook.name, "some disabled external service") - @patch('ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', - new=MockSSRFProtect._get_ip_address) + @patch( + 'ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', + new=MockSSRFProtect._get_ip_address + ) @responses.activate def test_send_and_retry(self): first_log_response = self._send_and_fail() # Let's retry through API call - retry_url = reverse("hook-log-retry", kwargs={ - "parent_lookup_asset": self.asset.uid, - "parent_lookup_hook": self.hook.uid, - "uid": first_log_response.get("uid") + retry_url = reverse('hook-log-retry', kwargs={ + 'parent_lookup_asset': self.asset.uid, + 'parent_lookup_hook': self.hook.uid, + 'uid': first_log_response.get('uid') }) # It should be a success @@ -188,17 +192,49 @@ def test_send_and_retry(self): self.assertEqual(response.status_code, status.HTTP_200_OK) # Let's check if logs has 2 tries - detail_url = reverse("hook-log-detail", kwargs={ - "parent_lookup_asset": self.asset.uid, - "parent_lookup_hook": self.hook.uid, - "uid": first_log_response.get("uid") + detail_url = reverse('hook-log-detail', kwargs={ + 'parent_lookup_asset': self.asset.uid, + 'parent_lookup_hook': self.hook.uid, + 'uid': first_log_response.get('uid') }) response = self.client.get(detail_url, format=SUBMISSION_FORMAT_TYPE_JSON) - self.assertEqual(response.data.get("tries"), 2) + self.assertEqual(response.data.get('tries'), 2) - @patch('ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', - new=MockSSRFProtect._get_ip_address) + @patch( + 'ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', + new=MockSSRFProtect._get_ip_address + ) + @responses.activate + def test_send_and_cannot_retry(self): + + first_log_response = self._send_and_wait_for_retry() + + # Let's retry through API call + retry_url = reverse('hook-log-retry', kwargs={ + 'parent_lookup_asset': self.asset.uid, + 'parent_lookup_hook': self.hook.uid, + 'uid': first_log_response.get('uid') + }) + + # It should be a failure. The hook log is going to be retried + response = self.client.patch(retry_url, format=SUBMISSION_FORMAT_TYPE_JSON) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + + # Let's check if logs has 2 tries + detail_url = reverse('hook-log-detail', kwargs={ + 'parent_lookup_asset': self.asset.uid, + 'parent_lookup_hook': self.hook.uid, + 'uid': first_log_response.get('uid') + }) + + response = self.client.get(detail_url, format=SUBMISSION_FORMAT_TYPE_JSON) + self.assertEqual(response.data.get('tries'), 1) + + @patch( + 'ssrf_protect.ssrf_protect.SSRFProtect._get_ip_address', + new=MockSSRFProtect._get_ip_address + ) @responses.activate def test_payload_template(self): @@ -321,12 +357,17 @@ def test_hook_log_filter_success(self): @responses.activate def test_hook_log_filter_failure(self): # Create failing hook - hook = self._create_hook(name="failing hook", - endpoint="http://failing.service.local/", - settings={}) - responses.add(responses.POST, hook.endpoint, - status=status.HTTP_500_INTERNAL_SERVER_ERROR, - content_type="application/json") + hook = self._create_hook( + name='failing hook', + endpoint='http://failing.service.local/', + settings={}, + ) + responses.add( + responses.POST, + hook.endpoint, + status=status.HTTP_504_GATEWAY_TIMEOUT, + content_type="application/json", + ) # simulate a submission ServiceDefinition = hook.get_service_definition() @@ -334,8 +375,8 @@ def test_hook_log_filter_failure(self): submission_id = submissions[0]['_id'] service_definition = ServiceDefinition(hook, submission_id) - success = service_definition.send() - self.assertFalse(success) + with pytest.raises(HookRemoteServerDownError): + service_definition.send() # Get log for the failing hook hook_log_url = reverse('hook-log-list', kwargs={ @@ -344,18 +385,24 @@ def test_hook_log_filter_failure(self): }) # There should be no success log for the failing hook - response = self.client.get(f'{hook_log_url}?status={HOOK_LOG_SUCCESS}', format='json') + response = self.client.get( + f'{hook_log_url}?status={HOOK_LOG_SUCCESS}', format='json' + ) self.assertEqual(response.data.get('count'), 0) # There should be a pending log for the failing hook - response = self.client.get(f'{hook_log_url}?status={HOOK_LOG_PENDING}', format='json') + response = self.client.get( + f'{hook_log_url}?status={HOOK_LOG_PENDING}', format='json' + ) self.assertEqual(response.data.get('count'), 1) def test_hook_log_filter_validation(self): # Create hook - hook = self._create_hook(name="success hook", - endpoint="http://hook.service.local/", - settings={}) + hook = self._create_hook( + name='success hook', + endpoint='http://hook.service.local/', + settings={}, + ) # Get log for the success hook hook_log_url = reverse('hook-log-list', kwargs={ diff --git a/kobo/apps/hook/tests/test_ssrf.py b/kobo/apps/hook/tests/test_ssrf.py index 89a71f4d4c..3d28e4a766 100644 --- a/kobo/apps/hook/tests/test_ssrf.py +++ b/kobo/apps/hook/tests/test_ssrf.py @@ -1,12 +1,13 @@ -# coding: utf-8 +import pytest import responses from constance.test import override_config from mock import patch from rest_framework import status +from ssrf_protect.exceptions import SSRFProtectException from kobo.apps.hook.constants import ( - HOOK_LOG_PENDING, + HOOK_LOG_FAILED, KOBO_INTERNAL_ERROR_STATUS_CODE ) from .hook_test_case import HookTestCase, MockSSRFProtect @@ -34,9 +35,10 @@ def test_send_with_ssrf_options(self): content_type='application/json') # Try to send data to external endpoint - success = service_definition.send() - self.assertFalse(success) + with pytest.raises(SSRFProtectException): + service_definition.send() + hook_log = hook.logs.all()[0] self.assertEqual(hook_log.status_code, KOBO_INTERNAL_ERROR_STATUS_CODE) - self.assertEqual(hook_log.status, HOOK_LOG_PENDING) + self.assertEqual(hook_log.status, HOOK_LOG_FAILED) self.assertTrue('is not allowed' in hook_log.message)