From 722dac86d558af58c1aed27ad9d2b71bfee946ae Mon Sep 17 00:00:00 2001 From: Douglas Anger Date: Tue, 15 May 2018 15:50:47 -0400 Subject: [PATCH] Fix crash when email submission endpoint unavailable (#6002) * Fix crash when email submission endpoint unavailable Handle KeyError and ValueError so that if the email submission endpoint goes down, Certbot can still run. Add tests to eff_test.py: - simulate non-JSON response as described in issue #5858 - simulate JSON response without 'status' element Non-JSON response throws an uncaught ValueError when attempting to decode as JSON. A JSON response missing the 'status' element throws an uncaught KeyError when checking whether status is True or False. Teach _check_response to handle ValueError and KeyError and report an issue to the user. Rewrite if statement as assertion with try-except block to make error handling consistent within the function. Update test_not_ok to make mocked raise_for_status function raise a requests.exceptions.HTTPError. Resolves #5858 * Update PR with requested changes - Use `if` instead of `assert` to check `status` element of response JSON - Handle KeyError and ValueError in the same way - Import requests at the beginning of eff_test.py - Clear JSON in test case in a more idiomatic way --- certbot/eff.py | 9 ++++++--- certbot/tests/eff_test.py | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/certbot/eff.py b/certbot/eff.py index 746261faa..b047c0b97 100644 --- a/certbot/eff.py +++ b/certbot/eff.py @@ -71,11 +71,14 @@ def _check_response(response): """ logger.debug('Received response:\n%s', response.content) - if response.ok: - if not response.json()['status']: + try: + response.raise_for_status() + if response.json()['status'] == False: _report_failure('your e-mail address appears to be invalid') - else: + except requests.exceptions.HTTPError: _report_failure() + except (ValueError, KeyError): + _report_failure('there was a problem with the server response') def _report_failure(reason=None): diff --git a/certbot/tests/eff_test.py b/certbot/tests/eff_test.py index 160af1993..8d0d5778c 100644 --- a/certbot/tests/eff_test.py +++ b/certbot/tests/eff_test.py @@ -1,4 +1,5 @@ """Tests for certbot.eff.""" +import requests import unittest import mock @@ -118,11 +119,28 @@ class SubscribeTest(unittest.TestCase): @test_util.patch_get_utility() def test_not_ok(self, mock_get_utility): self.response.ok = False + self.response.raise_for_status.side_effect = requests.exceptions.HTTPError self._call() # pylint: disable=no-value-for-parameter actual = self._get_reported_message(mock_get_utility) unexpected_part = 'because' self.assertFalse(unexpected_part in actual) + @test_util.patch_get_utility() + def test_response_not_json(self, mock_get_utility): + self.response.json.side_effect = ValueError() + self._call() # pylint: disable=no-value-for-parameter + actual = self._get_reported_message(mock_get_utility) + expected_part = 'problem' + self.assertTrue(expected_part in actual) + + @test_util.patch_get_utility() + def test_response_json_missing_status_element(self, mock_get_utility): + self.json.clear() + self._call() # pylint: disable=no-value-for-parameter + actual = self._get_reported_message(mock_get_utility) + expected_part = 'problem' + self.assertTrue(expected_part in actual) + def _get_reported_message(self, mock_get_utility): self.assertTrue(mock_get_utility().add_message.called) return mock_get_utility().add_message.call_args[0][0]