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

@@ -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):

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]