Update and delete registration with account reuse (#6174)

* find the correct url when deactivating an acmev1 account on the acmev2 endpoint

* set regr in ClientNetwork.account after deactivating on the server

* update self.net.account

* move logic into update_registration

* return methods to their original order to please git

* factor out common code

* update test_fowarding to use a method that still gets forwarded

* add acme module test coverage

* pragma no cover on correct line

* use previous regr uri

* strip unnecessary items from regr before saving

* add explanation to main.py

* add extra check to client_test.py

* use empty dict instead of empty string to indicate lack of body that we save to disk

acme/acme/client.py

@@ -50,7 +50,6 @@ class ClientBase(object):  # pylint: disable=too-many-instance-attributes
     :ivar .ClientNetwork net: Client network.
     :ivar int acme_version: ACME protocol version. 1 or 2.
     """
-
     def __init__(self, directory, net, acme_version):
         """Initialize.
 
@@ -588,6 +587,30 @@ class ClientV2(ClientBase):
         self.net.account = regr
         return regr
 
+    def update_registration(self, regr, update=None):
+        """Update registration.
+
+        :param messages.RegistrationResource regr: Registration Resource.
+        :param messages.Registration update: Updated body of the
+            resource. If not provided, body will be taken from `regr`.
+
+        :returns: Updated Registration Resource.
+        :rtype: `.RegistrationResource`
+
+        """
+        # https://github.com/certbot/certbot/issues/6155
+        new_regr = self._get_v2_account(regr)
+        return super(ClientV2, self).update_registration(new_regr, update)
+
+    def _get_v2_account(self, regr):
+        self.net.account = None
+        only_existing_reg = regr.body.update(only_return_existing=True)
+        response = self._post(self.directory['newAccount'], only_existing_reg)
+        updated_uri = response.headers['Location']
+        new_regr = regr.update(uri=updated_uri)
+        self.net.account = new_regr
+        return new_regr
+
     def new_order(self, csr_pem):
         """Request a new Order object from the server.
 
@@ -910,6 +933,7 @@ class ClientNetwork(object):  # pylint: disable=too-many-instance-attributes
         if acme_version == 2:
             kwargs["url"] = url
             # newAccount and revokeCert work without the kid
+            # newAccount must not have kid
             if self.account is not None:
                 kwargs["kid"] = self.account["uri"]
         kwargs["key"] = self.key

acme/acme/client_test.py

@@ -139,7 +139,7 @@ class BackwardsCompatibleClientV2Test(ClientTestBase):
         client = self._init()
         self.assertEqual(client.directory, client.client.directory)
         self.assertEqual(client.key, KEY)
-        self.assertEqual(client.update_registration, client.client.update_registration)
+        self.assertEqual(client.deactivate_registration, client.client.deactivate_registration)
         self.assertRaises(AttributeError, client.__getattr__, 'nonexistent')
         self.assertRaises(AttributeError, client.__getattr__, 'new_account_and_tos')
         self.assertRaises(AttributeError, client.__getattr__, 'new_account')
@@ -270,6 +270,13 @@ class BackwardsCompatibleClientV2Test(ClientTestBase):
             client.revoke(messages_test.CERT, self.rsn)
         mock_client().revoke.assert_called_once_with(messages_test.CERT, self.rsn)
 
+    def test_update_registration(self):
+        self.response.json.return_value = DIRECTORY_V1.to_json()
+        with mock.patch('acme.client.Client') as mock_client:
+            client = self._init()
+            client.update_registration(mock.sentinel.regr, None)
+        mock_client().update_registration.assert_called_once_with(mock.sentinel.regr, None)
+
 
 class ClientTest(ClientTestBase):
     """Tests for acme.client.Client."""
@@ -789,6 +796,19 @@ class ClientV2Test(ClientTestBase):
         self.net.post.assert_called_once_with(
             self.directory["revokeCert"], mock.ANY, acme_version=2)
 
+    def test_update_registration(self):
+        # "Instance of 'Field' has no to_json/update member" bug:
+        # pylint: disable=no-member
+        self.response.headers['Location'] = self.regr.uri
+        self.response.json.return_value = self.regr.body.to_json()
+        self.assertEqual(self.regr, self.client.update_registration(self.regr))
+        self.assertNotEqual(self.client.net.account, None)
+        self.assertEqual(self.client.net.post.call_count, 2)
+        self.assertTrue(DIRECTORY_V2.newAccount in self.net.post.call_args_list[0][0])
+
+        self.response.json.return_value = self.regr.body.update(
+            contact=()).to_json()
+
 
 class MockJSONDeSerializable(jose.JSONDeSerializable):
     # pylint: disable=missing-docstring

acme/acme/messages.py

@@ -274,6 +274,7 @@ class Registration(ResourceBody):
     agreement = jose.Field('agreement', omitempty=True)
     status = jose.Field('status', omitempty=True)
     terms_of_service_agreed = jose.Field('termsOfServiceAgreed', omitempty=True)
+    only_return_existing = jose.Field('onlyReturnExisting', omitempty=True)
 
     phone_prefix = 'tel:'
     email_prefix = 'mailto:'

certbot/account.py

@@ -270,9 +270,12 @@ class AccountFileStorage(interfaces.AccountStorage):
                 if hasattr(acme.directory, "new-authz"):
                     regr = RegistrationResourceWithNewAuthzrURI(
                         new_authzr_uri=acme.directory.new_authz,
-                        body=regr.body,
-                        uri=regr.uri,
-                        terms_of_service=regr.terms_of_service)
+                        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),

certbot/main.py

@@ -735,8 +735,13 @@ def register(config, unused_plugins):
     cb_client = client.Client(config, acc, None, None, acme=acme)
     # 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)))
+    # A v1 account being used as a v2 account will result in changing the uri to
+    # 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)
     add_msg("Your e-mail address was updated to {0}.".format(config.email))