Merge lp:~facundo/canonical-identity-provider/remove-refresh-with-both-macaroons into lp:canonical-identity-provider/release

[Facundo Batista]

Removed the deprecated way of refreshing macaroons (sending both root and discharge).

Removed the deprecated way of refreshing macaroons (sending both root and discharge).

[Matias Bordese (matiasb)]

LGTM

review: Approve

=== modified file 'src/api/v20/handlers.py'
--- src/api/v20/handlers.py	2016-07-27 14:41:55 +0000
+++ src/api/v20/handlers.py	2016-08-11 22:17:29 +0000
@@ -556,19 +556,8 @@
             missing = {'discharge_macaroon': [FIELD_REQUIRED]}
             return errors.INVALID_DATA(**missing)
 
-        if 'root_macaroon' in data:
-            # (deprecated 2016-05-03) use the root macaroon to old-fashion
-            # refresh, that also returns a bound discharge
-            method = auth.refresh_macaroons
-            params = (data['root_macaroon'], discharge_macaroon_raw)
-            stats.increment('macaroon.refresh', key='root_macaroon')
-        else:
-            method = auth.refresh_discharge
-            params = (discharge_macaroon_raw,)
-            stats.increment('macaroon.refresh', key='just_discharge')
-
         try:
-            new_discharge = method(*params)
+            new_discharge = auth.refresh_discharge(discharge_macaroon_raw)
         except ValidationError:
             return errors.INVALID_DATA()
         except AccountDeactivated:

=== modified file 'src/api/v20/tests/test_handlers.py'
--- src/api/v20/tests/test_handlers.py	2016-08-08 16:57:28 +0000
+++ src/api/v20/tests/test_handlers.py	2016-08-11 22:17:29 +0000
@@ -2850,22 +2850,6 @@
                                  expected_status_code=401,
                                  check_login_failed=False)
 
-    def test_macaroon_refreshed(self):
-        data = {'root_macaroon': self.root_macaroon.serialize(),
-                'discharge_macaroon': self.discharge_macaroon.serialize()}
-        json_body = self.do_post(data=data)
-
-        # get new discharge macaroon and verify with old root (its internals
-        # are verified by the tests of the 'refresh_macaroons' function itself)
-        discharge_macaroon_raw = json_body['discharge_macaroon']
-        discharge_macaroon = Macaroon.deserialize(discharge_macaroon_raw)
-        v = Verifier()
-        v.satisfy_general(lambda c: True)
-        v.verify(
-            self.root_macaroon, self.macaroon_random_key, [discharge_macaroon])
-        self.stats_increment.assert_any_call(
-            'macaroon.refresh', key='root_macaroon')
-
     def test_just_discharge_refreshed(self):
         json_body = self.do_post()
 
@@ -2878,5 +2862,3 @@
         v = Verifier()
         v.satisfy_general(lambda c: True)
         v.verify(self.root_macaroon, self.macaroon_random_key, [discharge])
-        self.stats_increment.assert_any_call(
-            'macaroon.refresh', key='just_discharge')

=== modified file 'src/identityprovider/auth.py'
--- src/identityprovider/auth.py	2016-07-28 16:07:45 +0000
+++ src/identityprovider/auth.py	2016-08-11 22:17:29 +0000
@@ -667,37 +667,6 @@
     return account, info_holder
 
 
-def refresh_macaroons(root_macaroon_raw, discharge_macaroon_raw):
-    """Refresh a root/discharge pair with a new discharge macaroon.
-
-    This function is deprecated; will be removed when clients stop hitting
-    the handler passing the root macaroon.
-    """
-    try:
-        root_macaroon = Macaroon.deserialize(root_macaroon_raw)
-        discharge_macaroon = Macaroon.deserialize(discharge_macaroon_raw)
-    except:
-        raise ValidationError("The received Macaroons are corrupt")
-
-    # get the raw caveat id and the decrypted deserialized info
-    raw_caveat_id, caveat_info = _get_own_caveat(root_macaroon)
-
-    account, macaroon_info = _account_from_macaroon(
-        root_macaroon, caveat_info['roothash'], discharge_macaroon)
-
-    # create the new discharge macaroon with same location, key and
-    # identifier than it's original 3rd-party caveat (so they can
-    # be matched and verified), and keeping the last_auth and expires from
-    # the original discharge macaroon
-    d = _build_discharge(
-        caveat_info['3rdparty'], raw_caveat_id, account,
-        last_auth=macaroon_info['last_auth'], expires=macaroon_info['expires'])
-
-    # return the properly prepared discharge macaroon
-    discharge = root_macaroon.prepare_for_request(d)
-    return discharge
-
-
 def refresh_discharge(discharge_macaroon_raw):
     """Refresh a discharge with a new discharge macaroon."""
     try:

=== modified file 'src/identityprovider/tests/test_auth.py'
--- src/identityprovider/tests/test_auth.py	2016-08-01 14:17:32 +0000
+++ src/identityprovider/tests/test_auth.py	2016-08-11 22:17:29 +0000
@@ -34,7 +34,6 @@
     build_discharge_macaroon,
     build_discharge_macaroon_from_root,
     refresh_discharge,
-    refresh_macaroons,
     validate_oauth_signature,
 )
 from identityprovider.login import AuthenticationError, AccountDeactivated
@@ -1175,135 +1174,6 @@
         self.assertRaises(ValidationError, _decrypt_caveat, caveat_id)
 
 
-class MacaroonRefreshFromRootTestCase(SSOBaseTestCase):
-    """Test the deprecated refresh_macaroons.
-
-    These tests are kept separated as that function won't evolve no more.
-    """
-
-    def setUp(self):
-        super(MacaroonRefreshFromRootTestCase, self).setUp()
-        self.root_macaroon, self.macaroon_random_key, _ = self.build_macaroon(
-            version=0)
-
-        # discharge the test macaroon
-        self.account = self.factory.make_account()
-        self.discharge_macaroon = build_discharge_macaroon_from_root(
-            self.account, self.root_macaroon.serialize())
-
-    def test_root_macaroon_corrupt(self):
-        self.assertRaises(
-            ValidationError, refresh_macaroons,
-            "Corrupted macaroon", self.discharge_macaroon.serialize())
-
-    def test_discharge_macaroon_corrupt(self):
-        self.assertRaises(
-            ValidationError, refresh_macaroons,
-            self.root_macaroon.serialize(), "Seriously corrupted macaroon")
-
-    def test_macaroons_dont_verify_ok(self):
-        # just get *another* discharge so it's not for the same root macaroon
-        other_root, _, _ = self.build_macaroon(version=0)
-        other_discharge = build_discharge_macaroon_from_root(
-            self.account, other_root.serialize())
-        self.assertRaises(AuthenticationError, refresh_macaroons,
-                          self.root_macaroon.serialize(),
-                          other_discharge.serialize())
-
-    def test_deactivated_account(self):
-        self.account.deactivate()
-        self.assertRaises(AccountDeactivated, refresh_macaroons,
-                          self.root_macaroon.serialize(),
-                          self.discharge_macaroon.serialize())
-
-    def test_password_changed(self):
-        self.account.set_password("a new password")
-        self.assertRaises(AuthenticationError, refresh_macaroons,
-                          self.root_macaroon.serialize(),
-                          self.discharge_macaroon.serialize())
-
-    def test_password_with_no_datetime(self):
-        # simulate an "old" account password (before we started to keep the
-        # changed date); note that I'm bypassing the AccountPassword save(),
-        # which will update the attribute that I want in Null! (and we confirm
-        # that in the assert)
-        self.account.accountpassword.date_changed = None
-        super(AccountPassword, self.account.accountpassword).save()
-        assert self.account.accountpassword.date_changed is None
-
-        # check that auths ok
-        refresh_macaroons(self.root_macaroon.serialize(),
-                          self.discharge_macaroon.serialize())
-
-    def test_proper_refreshing(self):
-        old_discharge = self.discharge_macaroon  # just rename for readability
-        service_location = settings.MACAROON_SERVICE_LOCATION
-
-        def get_value(search_key):
-            for caveat in old_discharge.first_party_caveats():
-                source, key, value = caveat.caveat_id.split("|", 2)
-                if source == service_location and key == search_key:
-                    return value
-
-        # get old values from the macaroon and also change the account to see
-        # that reflected
-        old_last_auth = get_value('last_auth')
-        old_expires = get_value('expires')
-        new_mail = self.factory.make_email_for_account(
-            self.account, status=EmailStatus.PREFERRED)
-        self.account.displayname = "New test display name"
-        self.account.save()
-
-        # call!
-        before = now()
-        new_discharge = refresh_macaroons(self.root_macaroon.serialize(),
-                                          old_discharge.serialize())
-        after = now()
-
-        # test
-        def checker(caveat):
-            """Assure all caveats inside the discharged macaroon are ok."""
-            source, key, value = caveat.split("|", 2)
-            if source != service_location:
-                # just checking the NEW discharge one, not the root
-                return True
-
-            if key == 'valid_since':
-                valid_since = datetime.strptime(value, '%Y-%m-%dT%H:%M:%S.%f')
-                self.assertGreater(valid_since, before)
-                self.assertGreater(after, valid_since)
-                return True
-
-            if key == 'last_auth':
-                self.assertEqual(value, old_last_auth)
-                return True
-
-            if key == 'account':
-                expected = {
-                    'openid': self.account.openid_identifier,
-                    'email': new_mail.email,
-                    'displayname':  "New test display name",
-                    'is_verified': self.account.is_verified,
-                    'username': self.account.username
-                }
-                actual = json.loads(base64.b64decode(value).decode("utf8"))
-                self.assertEqual(expected, actual)
-                return True
-
-            if key == 'expires':
-                self.assertEqual(value, old_expires)
-                return True
-
-            # we're not validating an SSO from the discharged macaroon, fail!
-            return False
-
-        # verify using the NEW discharge macaroon
-        v = Verifier()
-        v.satisfy_general(checker)
-        v.verify(self.root_macaroon, self.macaroon_random_key,
-                 [new_discharge])
-
-
 class MacaroonRefreshTestCase(SSOBaseTestCase):
 
     def setUp(self):
@@ -1386,7 +1256,7 @@
                 expected = {
                     'openid': self.account.openid_identifier,
                     'email': new_mail.email,
-                    'displayname':  "New test display name",
+                    'displayname': "New test display name",
                     'is_verified': self.account.is_verified,
                     'username': self.account.username
                 }