Support Register Unsafely in Update (#8212)

* Allow user to remove email using update command

Fixes #3162. Slight change to control flow to replace current email
addresses with an empty list. Also add appropriate result message when
an email is removed.

* Update ACME to allow update to remove fields

- New field type "UnFalseyField" that treats all non-None fields as
  non-empty
- Contact changed to new field type to allow sending of empty contact
  field
- Certbot update adjusted to use tuple instead of None when empty
- Test updated to check more logic
- Unrelated type hint added to keep pycharm gods happy

* Moved some mocks into decorators

* Restore default to `contact` but do not serialize

- Add `to_partial_json` and `fields_to_partial_json` to Registration
- Store private variable noting if the value of the `contact` field was
  provided by the user.
- Change message when updating without email to reflect removal of
  all contact info.
- Add note in changelog that `update_account` with the
  `--register-unsafely-without-email` flag will remove contact
  from an account.

* Reverse logic for field handling on serialization

Now forcably add contact when serilizing, but go back to base `jose`
field type.

* Responding to Review

- change out of date name
- update several comments
- update `from_data` function of `Registration`
- Update test to remove superfluous mock

* Responding to review

- Change comments to make from_data more clear
- Remove code worried about None (omitempty has got my back)
- Update test to be more reliable
- Add typing import with comment to avoid pylint bug

AUTHORS.md

@@ -61,6 +61,7 @@ Authors
 * [Daniel Albers](https://github.com/AID)
 * [Daniel Aleksandersen](https://github.com/da2x)
 * [Daniel Convissor](https://github.com/convissor)
+* [Daniel "Drex" Drexler](https://github.com/aeturnum)
 * [Daniel Huang](https://github.com/dhuang)
 * [Dave Guarino](https://github.com/daguar)
 * [David cz](https://github.com/dave-cz)

acme/acme/messages.py

@@ -315,6 +315,9 @@ class Registration(ResourceBody):
     # on new-reg key server ignores 'key' and populates it based on
     # JWS.signature.combined.jwk
     key = jose.Field('key', omitempty=True, decoder=jose.JWK.from_json)
+    # Contact field implements special behavior to allow messages that clear existing
+    # contacts while not expecting the `contact` field when loading from json.
+    # This is implemented in the constructor and *_json methods.
     contact = jose.Field('contact', omitempty=True, default=())
     agreement = jose.Field('agreement', omitempty=True)
     status = jose.Field('status', omitempty=True)
@@ -327,24 +330,73 @@ class Registration(ResourceBody):
 
     @classmethod
     def from_data(cls, phone=None, email=None, external_account_binding=None, **kwargs):
-        """Create registration resource from contact details."""
+        """
+        Create registration resource from contact details.
+
+        The `contact` keyword being passed to a Registration object is meaningful, so
+        this function represents empty iterables in its kwargs by passing on an empty
+        `tuple`.
+        """
+
+        # Note if `contact` was in kwargs.
+        contact_provided = 'contact' in kwargs
+
+        # Pop `contact` from kwargs and add formatted email or phone numbers
         details = list(kwargs.pop('contact', ()))
         if phone is not None:
             details.append(cls.phone_prefix + phone)
         if email is not None:
             details.extend([cls.email_prefix + mail for mail in email.split(',')])
-        kwargs['contact'] = tuple(details)
+
+        # Insert formatted contact information back into kwargs
+        # or insert an empty tuple if `contact` provided.
+        if details or contact_provided:
+            kwargs['contact'] = tuple(details)
 
         if external_account_binding:
             kwargs['external_account_binding'] = external_account_binding
 
         return cls(**kwargs)
 
+    def __init__(self, **kwargs):
+        """Note if the user provides a value for the `contact` member."""
+        if 'contact' in kwargs:
+            # Avoid the __setattr__ used by jose.TypedJSONObjectWithFields
+            object.__setattr__(self, '_add_contact', True)
+        super(Registration, self).__init__(**kwargs)
+
     def _filter_contact(self, prefix):
         return tuple(
             detail[len(prefix):] for detail in self.contact  # pylint: disable=not-an-iterable
             if detail.startswith(prefix))
 
+    def _add_contact_if_appropriate(self, jobj):
+        """
+        The `contact` member of Registration objects should not be required when
+        de-serializing (as it would be if the Fields' `omitempty` flag were `False`), but
+        it should be included in serializations if it was provided.
+
+        :param jobj: Dictionary containing this Registrations' data
+        :type jobj: dict
+
+        :returns: Dictionary containing Registrations data to transmit to the server
+        :rtype: dict
+        """
+        if getattr(self, '_add_contact', False):
+            jobj['contact'] = self.encode('contact')
+
+        return jobj
+
+    def to_partial_json(self):
+        """Modify josepy.JSONDeserializable.to_partial_json()"""
+        jobj = super(Registration, self).to_partial_json()
+        return self._add_contact_if_appropriate(jobj)
+
+    def fields_to_partial_json(self):
+        """Modify josepy.JSONObjectWithFields.fields_to_partial_json()"""
+        jobj = super(Registration, self).fields_to_partial_json()
+        return self._add_contact_if_appropriate(jobj)
+
     @property
     def phones(self):
         """All phones found in the ``contact`` field."""

acme/tests/messages_test.py

@@ -254,6 +254,19 @@ class RegistrationTest(unittest.TestCase):
         from acme.messages import Registration
         hash(Registration.from_json(self.jobj_from))
 
+    def test_default_not_transmitted(self):
+        from acme.messages import NewRegistration
+        empty_new_reg = NewRegistration()
+        new_reg_with_contact = NewRegistration(contact=())
+
+        self.assertEqual(empty_new_reg.contact, ())
+        self.assertEqual(new_reg_with_contact.contact, ())
+
+        self.assertTrue('contact' not in empty_new_reg.to_partial_json())
+        self.assertTrue('contact' not in empty_new_reg.fields_to_partial_json())
+        self.assertTrue('contact' in new_reg_with_contact.to_partial_json())
+        self.assertTrue('contact' in new_reg_with_contact.fields_to_partial_json())
+
 
 class UpdateRegistrationTest(unittest.TestCase):
     """Tests for acme.messages.UpdateRegistration."""

certbot/CHANGELOG.md

@@ -27,6 +27,8 @@ More details about these changes can be found on our GitHub repo.
   this concerns the plugin name, CLI flags, and keys in credential files.
   The prefixed form is still supported but is deprecated, and will be removed in a future release.
 * Added `--nginx-sleep-seconds` (default `1`) for environments where nginx takes a long time to reload.
+* Added the ability to remove email and phone contact information from an account 
+  using `update_account --register-unsafely-without-email`
 
 ### Changed
 
@@ -37,7 +39,8 @@ More details about these changes can be found on our GitHub repo.
 
 ### Fixed
 
-*
+* The `acme` library can now tell the ACME server to clear contact information by passing an empty
+  `tuple` to the `contact` field of a `Registration` message. 
 
 More details about these changes can be found on our GitHub repo.
 

certbot/certbot/_internal/main.py

@@ -11,7 +11,7 @@ import josepy as jose
 import zope.component
 
 from acme import errors as acme_errors
-from acme.magic_typing import Union
+from acme.magic_typing import Union, Iterable, Optional  # pylint: disable=unused-import
 import certbot
 from certbot import crypto_util
 from certbot import errors
@@ -590,7 +590,7 @@ def _init_le_client(config, authenticator, installer):
     :type config: interfaces.IConfig
 
     :param authenticator: Acme authentication handler
-    :type authenticator: interfaces.IAuthenticator
+    :type authenticator: Optional[interfaces.IAuthenticator]
     :param installer: Installer object
     :type installer: interfaces.IInstaller
 
@@ -703,17 +703,17 @@ def update_account(config, unused_plugins):
 
     if not accounts:
         return "Could not find an existing account to update."
-    if config.email is None:
-        if config.register_unsafely_without_email:
-            return ("--register-unsafely-without-email provided, however, a "
-                    "new e-mail address must\ncurrently be provided when "
-                    "updating a registration.")
+    if config.email is None and not config.register_unsafely_without_email:
         config.email = display_ops.get_email(optional=False)
 
     acc, acme = _determine_account(config)
     cb_client = client.Client(config, acc, None, None, acme=acme)
+    # Empty list of contacts in case the user is removing all emails
+
+    acc_contacts = () # type: Iterable[str]
+    if config.email:
+        acc_contacts = ['mailto:' + email for email in config.email.split(',')]
     # We rely on an exception to interrupt this process if it didn't work.
-    acc_contacts = ['mailto:' + email for email in config.email.split(',')]
     prev_regr_uri = acc.regr.uri
     acc.regr = cb_client.acme.update_registration(acc.regr.update(
         body=acc.regr.body.update(contact=acc_contacts)))
@@ -722,8 +722,13 @@ def update_account(config, unused_plugins):
     # so that we can also continue to use the account object with acmev1.
     acc.regr = acc.regr.update(uri=prev_regr_uri)
     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))
+
+    if config.email is None:
+        add_msg("Any contact information associated with this account has been removed.")
+    else:
+        eff.prepare_subscription(config, acc)
+        add_msg("Your e-mail address was updated to {0}.".format(config.email))
+
     return None
 
 

certbot/tests/main_test.py

@@ -1404,6 +1404,43 @@ class MainTest(test_util.ConfigTestCase):
                  "user@example.org"])
             self.assertTrue("Could not find an existing account" in x[0])
 
+    @mock.patch('certbot._internal.main._determine_account')
+    @mock.patch('certbot._internal.eff.prepare_subscription')
+    @mock.patch('certbot._internal.main.account')
+    def test_update_account_remove_email(self, mocked_account_module, mock_prepare, mock_det_acc):
+        # Mock account storage and the account object returned
+        mocked_storage = mock.MagicMock()
+        mocked_account = mock.MagicMock()
+
+        mocked_account_module.AccountFileStorage.return_value = mocked_storage
+        mocked_storage.find_all.return_value = [mocked_account]
+        mock_det_acc.return_value = (mocked_account, "foo")
+
+        # Mock registration body to verify calls are made
+        mock_regr_body = mock.MagicMock()
+
+        # mocked_account.regr is overwritten in update, requiring an odd mock setup
+        mocked_account.regr.body = mock_regr_body
+
+        x = self._call(
+            ["update_account", "--register-unsafely-without-email"])
+
+
+        # When update succeeds, the return value of update_account() is None
+        self.assertTrue(x[0] is None)
+        # and we got supposedly did update the registration from
+        # the server
+        client_mock = x[3]
+        self.assertTrue(client_mock.Client().acme.update_registration.called)
+
+        self.assertTrue(mock_regr_body.update.called)
+        self.assertTrue('contact' in mock_regr_body.update.call_args[1])
+        self.assertEqual(mock_regr_body.update.call_args[1]['contact'], ())
+        # and we saved the updated registration on disk
+        self.assertTrue(mocked_storage.update_regr.called)
+        # ensure we didn't try to subscribe (no email to subscribe with)
+        self.assertFalse(mock_prepare.called)
+
     @mock.patch('certbot._internal.main.display_ops.get_email')
     @test_util.patch_get_utility()
     def test_update_account_with_email(self, mock_utility, mock_email):