From 20be8b327d5cb19b2ef5dedae3b771c1de952bd3 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 31 Jan 2017 17:08:21 -0800 Subject: [PATCH] Provide a way to opt-in to EFF e-mail (#4082) * Add eff email flags * add eff_sign_up * add requests dep to certbot * make pylint happy * Add EFF subscribe uri * add POST to EFF and write tests * log EFF e-mail submission * Add eff module and tests * cleanup client tests * offer subscription when changing e-mail * cleanup client.py and tests * expand e-mail prompt --- certbot/cli.py | 6 ++ certbot/client.py | 9 ++- certbot/constants.py | 3 + certbot/eff.py | 95 ++++++++++++++++++++++++ certbot/main.py | 2 + certbot/tests/client_test.py | 56 +++++++++------ certbot/tests/eff_test.py | 135 +++++++++++++++++++++++++++++++++++ certbot/tests/main_test.py | 7 +- setup.py | 3 + 9 files changed, 288 insertions(+), 28 deletions(-) create mode 100644 certbot/eff.py create mode 100644 certbot/tests/eff_test.py diff --git a/certbot/cli.py b/certbot/cli.py index f3b07d925..a24f566d1 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -880,6 +880,12 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis helpful.add( ["register", "unregister", "automation"], "-m", "--email", help=config_help("email")) + helpful.add(["register", "automation"], "--eff-email", action="store_true", + default=None, dest="eff_email", + help="Share your e-mail address with EFF") + helpful.add(["register", "automation"], "--no-eff-email", action="store_false", + default=None, dest="eff_email", + help="Don't share your e-mail address with EFF") helpful.add( ["automation", "certonly", "run"], "--keep-until-expiring", "--keep", "--reinstall", diff --git a/certbot/client.py b/certbot/client.py index 8d53a29d3..26c5e87a9 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -15,15 +15,16 @@ import certbot from certbot import account from certbot import auth_handler +from certbot import cli from certbot import constants from certbot import crypto_util -from certbot import errors +from certbot import eff from certbot import error_handler +from certbot import errors from certbot import interfaces -from certbot import util from certbot import reverter from certbot import storage -from certbot import cli +from certbot import util from certbot.display import ops as display_ops from certbot.display import enhancements @@ -139,6 +140,8 @@ def register(config, account_storage, tos_cb=None): account.report_new_account(acc, config) account_storage.save(acc) + eff.handle_subscription(config) + return acc, acme diff --git a/certbot/constants.py b/certbot/constants.py index f64ff7e1e..b286ca26a 100644 --- a/certbot/constants.py +++ b/certbot/constants.py @@ -106,3 +106,6 @@ RENEWAL_CONFIGS_DIR = "renewal" FORCE_INTERACTIVE_FLAG = "--force-interactive" """Flag to disable TTY checking in IDisplay.""" + +EFF_SUBSCRIBE_URI = "https://supporters.eff.org/subscribe/certbot" +"""EFF URI used to submit the e-mail address of users who opt-in.""" diff --git a/certbot/eff.py b/certbot/eff.py new file mode 100644 index 000000000..0ef23cc8c --- /dev/null +++ b/certbot/eff.py @@ -0,0 +1,95 @@ +"""Subscribes users to the EFF newsletter.""" +import logging + +import requests +import zope.component + +from certbot import constants +from certbot import interfaces + + +logger = logging.getLogger(__name__) + + +def handle_subscription(config): + """High level function to take care of EFF newsletter subscriptions. + + The user may be asked if they want to sign up for the newsletter if + they have not already specified. + + :param .IConfig config: Client configuration. + + """ + if config.email is None: + if config.eff_email: + _report_failure("you didn't provide an e-mail address") + return + if config.eff_email is None: + config.eff_email = _want_subscription() + if config.eff_email: + subscribe(config.email) + + +def _want_subscription(): + """Does the user want to be subscribed to the EFF newsletter? + + :returns: True if we should subscribe the user, otherwise, False + :rtype: bool + + """ + prompt = ( + 'Would you be willing to share your email address with the ' + "Electronic Frontier Foundation, a founding partner of the Let's " + 'Encrypt project and the non-profit organization that develops ' + "Cerbot? We'd like to send you email about EFF and our work to " + 'encrypt the web, protect its users and defend digital rights.') + display = zope.component.getUtility(interfaces.IDisplay) + return display.yesno(prompt, default=False) + + +def subscribe(email): + """Subscribe the user to the EFF mailing list. + + :param str email: the e-mail address to subscribe + + """ + url = constants.EFF_SUBSCRIBE_URI + data = {'data_type': 'json', + 'email': email, + 'form_id': 'eff_supporters_library_subscribe_form'} + logger.debug('Sending POST request to %s:\n%s', url, data) + _check_response(requests.post(url, data=data)) + + +def _check_response(response): + """Check for errors in the server's response. + + If an error occurred, it will be reported to the user. + + :param requests.Response response: the server's response to the + subscription request + + """ + logger.debug('Received response:\n%s', response.content) + if response.ok: + if not response.json()['status']: + _report_failure('your e-mail address appears to be invalid') + else: + _report_failure() + + +def _report_failure(reason=None): + """Notify the user of failing to sign them up for the newsletter. + + :param reason: a phrase describing what the problem was + beginning with a lowercase letter and no closing punctuation + :type reason: `str` or `None` + + """ + msg = ['We were unable to subscribe you the EFF mailing list'] + if reason is not None: + msg.append(' because ') + msg.append(reason) + msg.append('. You can try again later by visiting https://act.eff.org.') + reporter = zope.component.getUtility(interfaces.IReporter) + reporter.add_message(''.join(msg), reporter.LOW_PRIORITY) diff --git a/certbot/main.py b/certbot/main.py index b52e605be..7ca346050 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -24,6 +24,7 @@ from certbot import crypto_util from certbot import colored_logging from certbot import configuration from certbot import constants +from certbot import eff from certbot import errors from certbot import hooks from certbot import interfaces @@ -474,6 +475,7 @@ def register(config, unused_plugins): acc.regr = acme_client.acme.update_registration(acc.regr.update( body=acc.regr.body.update(contact=('mailto:' + config.email,)))) account_storage.save_regr(acc) + eff.handle_subscription(config) add_msg("Your e-mail address was updated to {0}.".format(config.email)) diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index f4b86fc7c..cc3bb098d 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -44,20 +44,25 @@ class RegisterTest(unittest.TestCase): def test_no_tos(self): with mock.patch("certbot.client.acme_client.Client") as mock_client: mock_client.register().terms_of_service = "http://tos" - with mock.patch("certbot.account.report_new_account"): - self.tos_cb.return_value = False - self.assertRaises(errors.Error, self._call) + with mock.patch("certbot.eff.handle_subscription") as mock_handle: + with mock.patch("certbot.account.report_new_account"): + self.tos_cb.return_value = False + self.assertRaises(errors.Error, self._call) + self.assertFalse(mock_handle.called) - self.tos_cb.return_value = True - self._call() + self.tos_cb.return_value = True + self._call() + self.assertTrue(mock_handle.called) - self.tos_cb = None - self._call() + self.tos_cb = None + self._call() + self.assertEqual(mock_handle.call_count, 2) def test_it(self): with mock.patch("certbot.client.acme_client.Client"): with mock.patch("certbot.account.report_new_account"): - self._call() + with mock.patch("certbot.eff.handle_subscription"): + self._call() @mock.patch("certbot.account.report_new_account") @mock.patch("certbot.client.display_ops.get_email") @@ -67,9 +72,11 @@ class RegisterTest(unittest.TestCase): msg = "DNS problem: NXDOMAIN looking up MX for example.com" mx_err = messages.Error.with_code('invalidContact', detail=msg) with mock.patch("certbot.client.acme_client.Client") as mock_client: - mock_client().register.side_effect = [mx_err, mock.MagicMock()] - self._call() - self.assertEqual(mock_get_email.call_count, 1) + with mock.patch("certbot.eff.handle_subscription") as mock_handle: + mock_client().register.side_effect = [mx_err, mock.MagicMock()] + self._call() + self.assertEqual(mock_get_email.call_count, 1) + self.assertTrue(mock_handle.called) @mock.patch("certbot.account.report_new_account") def test_email_invalid_noninteractive(self, _rep): @@ -77,8 +84,9 @@ class RegisterTest(unittest.TestCase): msg = "DNS problem: NXDOMAIN looking up MX for example.com" mx_err = messages.Error.with_code('invalidContact', detail=msg) with mock.patch("certbot.client.acme_client.Client") as mock_client: - mock_client().register.side_effect = [mx_err, mock.MagicMock()] - self.assertRaises(errors.Error, self._call) + with mock.patch("certbot.eff.handle_subscription"): + mock_client().register.side_effect = [mx_err, mock.MagicMock()] + self.assertRaises(errors.Error, self._call) def test_needs_email(self): self.config.email = None @@ -86,21 +94,25 @@ class RegisterTest(unittest.TestCase): @mock.patch("certbot.client.logger") def test_without_email(self, mock_logger): - with mock.patch("certbot.client.acme_client.Client"): - with mock.patch("certbot.account.report_new_account"): - self.config.email = None - self.config.register_unsafely_without_email = True - self.config.dry_run = False - self._call() - mock_logger.warning.assert_called_once_with(mock.ANY) + with mock.patch("certbot.eff.handle_subscription") as mock_handle: + with mock.patch("certbot.client.acme_client.Client"): + with mock.patch("certbot.account.report_new_account"): + self.config.email = None + self.config.register_unsafely_without_email = True + self.config.dry_run = False + self._call() + mock_logger.warning.assert_called_once_with(mock.ANY) + self.assertTrue(mock_handle.called) def test_unsupported_error(self): from acme import messages msg = "Test" mx_err = messages.Error(detail=msg, typ="malformed", title="title") with mock.patch("certbot.client.acme_client.Client") as mock_client: - mock_client().register.side_effect = [mx_err, mock.MagicMock()] - self.assertRaises(messages.Error, self._call) + with mock.patch("certbot.eff.handle_subscription") as mock_handle: + mock_client().register.side_effect = [mx_err, mock.MagicMock()] + self.assertRaises(messages.Error, self._call) + self.assertFalse(mock_handle.called) class ClientTestCommon(unittest.TestCase): diff --git a/certbot/tests/eff_test.py b/certbot/tests/eff_test.py new file mode 100644 index 000000000..fd9a61181 --- /dev/null +++ b/certbot/tests/eff_test.py @@ -0,0 +1,135 @@ +"""Tests for certbot.eff.""" +import unittest + +import mock + +from certbot import constants +from certbot.tests import util + + +class HandleSubscriptionTest(unittest.TestCase): + """Tests for certbot.eff.handle_subscription.""" + def setUp(self): + self.email = 'certbot@example.org' + self.config = mock.Mock(email=self.email, eff_email=None) + + def _call(self): + from certbot.eff import handle_subscription + return handle_subscription(self.config) + + @util.patch_get_utility() + @mock.patch('certbot.eff.subscribe') + def test_failure(self, mock_subscribe, mock_get_utility): + self.config.email = None + self.config.eff_email = True + self._call() + self.assertFalse(mock_subscribe.called) + self.assertFalse(mock_get_utility().yesno.called) + actual = mock_get_utility().add_message.call_args[0][0] + expected_part = "because you didn't provide an e-mail address" + self.assertTrue(expected_part in actual) + + @mock.patch('certbot.eff.subscribe') + def test_no_subscribe_with_no_prompt(self, mock_subscribe): + self.config.eff_email = False + with util.patch_get_utility() as mock_get_utility: + self._call() + self.assertFalse(mock_subscribe.called) + self._assert_no_get_utility_calls(mock_get_utility) + + @util.patch_get_utility() + @mock.patch('certbot.eff.subscribe') + def test_subscribe_with_no_prompt(self, mock_subscribe, mock_get_utility): + self.config.eff_email = True + self._call() + self._assert_subscribed(mock_subscribe) + self._assert_no_get_utility_calls(mock_get_utility) + + def _assert_no_get_utility_calls(self, mock_get_utility): + self.assertFalse(mock_get_utility().yesno.called) + self.assertFalse(mock_get_utility().add_message.called) + + @util.patch_get_utility() + @mock.patch('certbot.eff.subscribe') + def test_subscribe_with_prompt(self, mock_subscribe, mock_get_utility): + mock_get_utility().yesno.return_value = True + self._call() + self._assert_subscribed(mock_subscribe) + self.assertFalse(mock_get_utility().add_message.called) + self._assert_correct_yesno_call(mock_get_utility) + + def _assert_subscribed(self, mock_subscribe): + self.assertTrue(mock_subscribe.called) + self.assertEqual(mock_subscribe.call_args[0][0], self.email) + + @util.patch_get_utility() + @mock.patch('certbot.eff.subscribe') + def test_no_subscribe_with_prompt(self, mock_subscribe, mock_get_utility): + mock_get_utility().yesno.return_value = False + self._call() + self.assertFalse(mock_subscribe.called) + self.assertFalse(mock_get_utility().add_message.called) + self._assert_correct_yesno_call(mock_get_utility) + + def _assert_correct_yesno_call(self, mock_get_utility): + self.assertTrue(mock_get_utility().yesno.called) + call_args, call_kwargs = mock_get_utility().yesno.call_args + actual = call_args[0] + expected_part = 'Electronic Frontier Foundation' + self.assertTrue(expected_part in actual) + self.assertFalse(call_kwargs.get('default', True)) + + +class SubscribeTest(unittest.TestCase): + """Tests for certbot.eff.subscribe.""" + def setUp(self): + self.email = 'certbot@example.org' + self.json = {'status': True} + self.response = mock.Mock(ok=True) + self.response.json.return_value = self.json + + @mock.patch('certbot.eff.requests.post') + def _call(self, mock_post): + mock_post.return_value = self.response + + from certbot.eff import subscribe + subscribe(self.email) + self._check_post_call(mock_post) + + def _check_post_call(self, mock_post): + self.assertEqual(mock_post.call_count, 1) + call_args, call_kwargs = mock_post.call_args + self.assertEqual(call_args[0], constants.EFF_SUBSCRIBE_URI) + + data = call_kwargs.get('data') + self.assertFalse(data is None) + self.assertEqual(data.get('email'), self.email) + + @util.patch_get_utility() + def test_bad_status(self, mock_get_utility): + self.json['status'] = False + self._call() # pylint: disable=no-value-for-parameter + actual = self._get_reported_message(mock_get_utility) + expected_part = 'because your e-mail address appears to be invalid.' + self.assertTrue(expected_part in actual) + + @util.patch_get_utility() + def test_not_ok(self, mock_get_utility): + self.response.ok = False + 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) + + 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] + + @util.patch_get_utility() + def test_subscribe(self, mock_get_utility): + self._call() # pylint: disable=no-value-for-parameter + self.assertFalse(mock_get_utility.called) + + +if __name__ == '__main__': + unittest.main() # pragma: no cover diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index a4f3a7805..22b709f69 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -1145,9 +1145,9 @@ class MainTest(unittest.TestCase): # pylint: disable=too-many-public-methods def test_update_registration_with_email(self, mock_utility, mock_email): email = "user@example.com" mock_email.return_value = email - with mock.patch('certbot.main.client') as mocked_client: - with mock.patch('certbot.main.account') as mocked_account: - with mock.patch('certbot.main._determine_account') as mocked_det: + with mock.patch('certbot.eff.handle_subscription') as mock_handle: + with mock.patch('certbot.main._determine_account') as mocked_det: + with mock.patch('certbot.main.account') as mocked_account: with mock.patch('certbot.main.client') as mocked_client: mocked_storage = mock.MagicMock() mocked_account.AccountFileStorage.return_value = mocked_storage @@ -1168,6 +1168,7 @@ class MainTest(unittest.TestCase): # pylint: disable=too-many-public-methods self.assertTrue(mocked_storage.save_regr.called) self.assertTrue( email in mock_utility().add_message.call_args[0][0]) + self.assertTrue(mock_handle.called) class UnregisterTest(unittest.TestCase): diff --git a/setup.py b/setup.py index a47aff11b..529459cf0 100644 --- a/setup.py +++ b/setup.py @@ -31,6 +31,9 @@ changes = read_file(os.path.join(here, 'CHANGES.rst')) version = meta['version'] # Please update tox.ini when modifying dependency version requirements +# This package relies on requests, however, it isn't specified here to avoid +# masking the more specific request requirements in acme. See +# https://github.com/pypa/pip/issues/988 for more info. install_requires = [ 'acme=={0}'.format(version), # We technically need ConfigArgParse 0.10.0 for Python 2.6 support, but