Deferred EFF subscription until the first certificate is successfully…

… issued (certbot#8076)

* Base logic

* Various controls when email is None

* Adapt eff tests

* Forward compatibility

* Also for csr

* Explicit regr or meta updates in account objects

* Adapt logic to ask for eff subscription during registering

* Adapt tests

* Move dry-run control

* Add some relevant controls on handle_subscription call checks

certbot/certbot/_internal/account.py

@@ -15,6 +15,7 @@
 
 from acme import fields as acme_fields
 from acme import messages
+from acme.client import ClientBase  # pylint: disable=unused-import
 from certbot import errors
 from certbot import interfaces
 from certbot import util
@@ -39,22 +40,25 @@ class Meta(jose.JSONObjectWithFields):
 
         :ivar datetime.datetime creation_dt: Creation date and time (UTC).
         :ivar str creation_host: FQDN of host, where account has been created.
+        :ivar str register_to_eff: If not None, Certbot will register the provided
+                                        email during the account registration.
 
         .. note:: ``creation_dt`` and ``creation_host`` are useful in
             cross-machine migration scenarios.
 
         """
         creation_dt = acme_fields.RFC3339Field("creation_dt")
         creation_host = jose.Field("creation_host")
+        register_to_eff = jose.Field("register_to_eff", omitempty=True)
 
     def __init__(self, regr, key, meta=None):
         self.key = key
         self.regr = regr
         self.meta = self.Meta(
             # pyrfc3339 drops microseconds, make sure __eq__ is sane
-            creation_dt=datetime.datetime.now(
-                tz=pytz.UTC).replace(microsecond=0),
-            creation_host=socket.getfqdn()) if meta is None else meta
+            creation_dt=datetime.datetime.now(tz=pytz.UTC).replace(microsecond=0),
+            creation_host=socket.getfqdn(),
+            register_to_eff=None) if meta is None else meta
 
         # try MD5, else use MD5 in non-security mode (e.g. for FIPS systems / RHEL)
         try:
@@ -242,15 +246,47 @@ def load(self, account_id):
         return self._load_for_server_path(account_id, self.config.server_path)
 
     def save(self, account, client):
-        self._save(account, client, regr_only=False)
+        # type: (Account, ClientBase) -> None
+        """Create a new account.
 
-    def save_regr(self, account, acme):
-        """Save the registration resource.
+        :param Account account: account to create
+        :param ClientBase client: ACME client associated to the account
 
-        :param Account account: account whose regr should be saved
+        """
+        try:
+            dir_path = self._prepare(account)
+            self._create(account, dir_path)
+            self._update_meta(account, dir_path)
+            self._update_regr(account, client, dir_path)
+        except IOError as error:
+            raise errors.AccountStorageError(error)
+
+    def update_regr(self, account, client):
+        # type: (Account, ClientBase) -> None
+        """Update the registration resource.
+
+        :param Account account: account to update
+        :param ClientBase client: ACME client associated to the account
+
+        """
+        try:
+            dir_path = self._prepare(account)
+            self._update_regr(account, client, dir_path)
+        except IOError as error:
+            raise errors.AccountStorageError(error)
+
+    def update_meta(self, account):
+        # type: (Account) -> None
+        """Update the meta resource.
+
+        :param Account account: account to update
 
         """
-        self._save(account, acme, regr_only=True)
+        try:
+            dir_path = self._prepare(account)
+            self._update_meta(account, dir_path)
+        except IOError as error:
+            raise errors.AccountStorageError(error)
 
     def delete(self, account_id):
         """Delete registration info from disk
@@ -318,32 +354,36 @@ def _delete_links_and_find_target_dir(self, server_path, link_func):
 
         return dir_path
 
-    def _save(self, account, acme, regr_only):
+    def _prepare(self, account):
+        # type: (Account) -> str
         account_dir_path = self._account_dir_path(account.id)
         util.make_or_verify_dir(account_dir_path, 0o700, self.config.strict_permissions)
-        try:
-            with open(self._regr_path(account_dir_path), "w") as regr_file:
-                regr = account.regr
-                # If we have a value for new-authz, save it for forwards
-                # compatibility with older versions of Certbot. If we don't
-                # have a value for new-authz, this is an ACMEv2 directory where
-                # an older version of Certbot won't work anyway.
-                if hasattr(acme.directory, "new-authz"):
-                    regr = RegistrationResourceWithNewAuthzrURI(
-                        new_authzr_uri=acme.directory.new_authz,
-                        body={},
-                        uri=regr.uri)
-                else:
-                    regr = messages.RegistrationResource(
-                        body={},
-                        uri=regr.uri)
-                regr_file.write(regr.json_dumps())
-            if not regr_only:
-                with util.safe_open(self._key_path(account_dir_path),
-                                    "w", chmod=0o400) as key_file:
-                    key_file.write(account.key.json_dumps())
-                with open(self._metadata_path(
-                        account_dir_path), "w") as metadata_file:
-                    metadata_file.write(account.meta.json_dumps())
-        except IOError as error:
-            raise errors.AccountStorageError(error)
+        return account_dir_path
+
+    def _create(self, account, dir_path):
+        # type: (Account, str) -> None
+        with util.safe_open(self._key_path(dir_path), "w", chmod=0o400) as key_file:
+            key_file.write(account.key.json_dumps())
+
+    def _update_regr(self, account, acme, dir_path):
+        # type: (Account, ClientBase, str) -> None
+        with open(self._regr_path(dir_path), "w") as regr_file:
+            regr = account.regr
+            # If we have a value for new-authz, save it for forwards
+            # compatibility with older versions of Certbot. If we don't
+            # have a value for new-authz, this is an ACMEv2 directory where
+            # an older version of Certbot won't work anyway.
+            if hasattr(acme.directory, "new-authz"):
+                regr = RegistrationResourceWithNewAuthzrURI(
+                    new_authzr_uri=acme.directory.new_authz,
+                    body={},
+                    uri=regr.uri)
+            else:
+                regr = messages.RegistrationResource(
+                    body={},
+                    uri=regr.uri)
+            regr_file.write(regr.json_dumps())
+
+    def _update_meta(self, account, dir_path):
+        with open(self._metadata_path(dir_path), "w") as metadata_file:
+            metadata_file.write(account.meta.json_dumps())

certbot/certbot/_internal/client.py

@@ -178,7 +178,7 @@ def register(config, account_storage, tos_cb=None):
     account.report_new_account(config)
     account_storage.save(acc, acme)
 
-    eff.handle_subscription(config)
+    eff.prepare_subscription(config, acc)
 
     return acc, acme
 
@@ -389,6 +389,7 @@ def _get_order_and_authorizations(self, csr_pem, best_effort):
 
         authzr = self.auth_handler.handle_authorizations(orderr, best_effort)
         return orderr.update(authorizations=authzr)
+
     def obtain_and_enroll_certificate(self, domains, certname):
         """Obtain and enroll certificate.
 

certbot/certbot/_internal/eff.py

@@ -4,49 +4,86 @@
 import requests
 import zope.component
 
+from acme.magic_typing import Optional  # pylint: disable=unused-import
+
 from certbot import interfaces
 from certbot._internal import constants
+from certbot._internal.account import Account  # pylint: disable=unused-import
+from certbot._internal.account import AccountFileStorage
+from certbot.interfaces import IConfig  # pylint: disable=unused-import
 
 logger = logging.getLogger(__name__)
 
 
-def handle_subscription(config):
-    """High level function to take care of EFF newsletter subscriptions.
+def prepare_subscription(config, acc):
+    # type: (IConfig, Account) -> None
+    """High level function to store potential EFF newsletter subscriptions.
 
     The user may be asked if they want to sign up for the newsletter if
-    they have not already specified.
+    they have not given their explicit approval or refusal using --eff-mail
+    or --no-eff-mail flag.
 
-    :param .IConfig config: Client configuration.
+    Decision about EFF subscription will be stored in the account metadata.
+
+    :param IConfig config: Client configuration.
+    :param Account acc: Current client account.
 
     """
-    if config.email is None:
-        if config.eff_email:
+    if config.eff_email is False:
+        return
+    if config.eff_email is True:
+        if config.email is None:
             _report_failure("you didn't provide an e-mail address")
+        else:
+            acc.meta = acc.meta.update(register_to_eff=config.email)
+    elif config.email and _want_subscription():
+        acc.meta = acc.meta.update(register_to_eff=config.email)
+
+    if acc.meta.register_to_eff:
+        storage = AccountFileStorage(config)
+        storage.update_meta(acc)
+
+
+def handle_subscription(config, acc):
+    # type: (IConfig, Account) -> None
+    """High level function to take care of EFF newsletter subscriptions.
+
+    Once subscription is handled, it will not be handled again.
+
+    :param IConfig config: Client configuration.
+    :param Account acc: Current client account.
+
+    """
+    if config.dry_run:
         return
-    if config.eff_email is None:
-        config.eff_email = _want_subscription()
-    if config.eff_email:
-        subscribe(config.email)
+    if acc.meta.register_to_eff:
+        subscribe(acc.meta.register_to_eff)
+
+        acc.meta = acc.meta.update(register_to_eff=None)
+        storage = AccountFileStorage(config)
+        storage.update_meta(acc)
 
 
 def _want_subscription():
+    # type: () -> bool
     """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 '
-        "Certbot? We'd like to send you email about our work encrypting "
+        'Would you be willing, once your first certificate is successfully issued, '
+        '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 Certbot? We'd like to send you email about our work encrypting "
         "the web, EFF news, campaigns, and ways to support digital freedom. ")
     display = zope.component.getUtility(interfaces.IDisplay)
     return display.yesno(prompt, default=False)
 
 
 def subscribe(email):
+    # type: (str) -> None
     """Subscribe the user to the EFF mailing list.
 
     :param str email: the e-mail address to subscribe
@@ -56,11 +93,13 @@ def subscribe(email):
     data = {'data_type': 'json',
             'email': email,
             'form_id': 'eff_supporters_library_subscribe_form'}
+    logger.info('Subscribe to the EFF mailing list (email: %s).', email)
     logger.debug('Sending POST request to %s:\n%s', url, data)
     _check_response(requests.post(url, data=data))
 
 
 def _check_response(response):
+    # type: (requests.Response) -> None
     """Check for errors in the server's response.
 
     If an error occurred, it will be reported to the user.
@@ -81,6 +120,7 @@ def _check_response(response):
 
 
 def _report_failure(reason=None):
+    # type: (Optional[str]) -> None
     """Notify the user of failing to sign them up for the newsletter.
 
     :param reason: a phrase describing what the problem was

certbot/certbot/_internal/main.py

@@ -721,11 +721,12 @@ def update_account(config, unused_plugins):
     # the v2 uri. Since it's the same object on disk, put it back to the v1 uri
     # so that we can also continue to use the account object with acmev1.
     acc.regr = acc.regr.update(uri=prev_regr_uri)
-    account_storage.save_regr(acc, cb_client.acme)
-    eff.handle_subscription(config)
+    account_storage.update_regr(acc, cb_client.acme)
+    eff.prepare_subscription(config, acc)
     add_msg("Your e-mail address was updated to {0}.".format(config.email))
     return None
 
+
 def _install_cert(config, le_client, domains, lineage=None):
     """Install a cert
 
@@ -1116,6 +1117,7 @@ def run(config, plugins):
         display_ops.success_renewal(domains)
 
     _suggest_donation_if_appropriate(config)
+    eff.handle_subscription(config, le_client.account)
     return None
 
 
@@ -1189,6 +1191,7 @@ def renew_cert(config, plugins, lineage):
         notify("new certificate deployed with reload of {0} server; fullchain is {1}".format(
                config.installer, lineage.fullchain), pause=False)
 
+
 def certonly(config, plugins):
     """Authenticate & obtain cert, but do not install it.
 
@@ -1220,6 +1223,7 @@ def certonly(config, plugins):
         cert_path, fullchain_path = _csr_get_and_save_cert(config, le_client)
         _report_new_cert(config, cert_path, fullchain_path)
         _suggest_donation_if_appropriate(config)
+        eff.handle_subscription(config, le_client.account)
         return
 
     domains, certname = _find_domains_or_certname(config, installer)
@@ -1237,6 +1241,8 @@ def certonly(config, plugins):
     key_path = lineage.key_path if lineage else None
     _report_new_cert(config, cert_path, fullchain_path, key_path)
     _suggest_donation_if_appropriate(config)
+    eff.handle_subscription(config, le_client.account)
+
 
 def renew(config, unused_plugins):
     """Renew previously-obtained certificates.