Merge lp:~facundo/canonical-identity-provider/no-caveat-in-refresh into lp:canonical-identity-provider/release

[Facundo Batista]

Don't need the caveat_id in the refresh.

Don't need the caveat_id in the refresh.

[Ricardo Kirkner (ricardokirkner)]

LGTM

review: Approve

[Matias Bordese (matiasb)]

LGTM

review: Approve

=== modified file 'src/api/v20/handlers.py'
--- src/api/v20/handlers.py	2016-05-05 11:39:43 +0000
+++ src/api/v20/handlers.py	2016-05-05 15:40:29 +0000
@@ -543,18 +543,16 @@
             missing = {'discharge_macaroon': [FIELD_REQUIRED]}
             return errors.INVALID_DATA(**missing)
 
-        if 'caveat_id' in data:
-            method = auth.refresh_discharge
-            params = (data['caveat_id'], discharge_macaroon_raw)
-            stats.increment('macaroon.refresh', key='caveat_id')
-        elif 'root_macaroon' in data:
+        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='macaroon')
+            stats.increment('macaroon.refresh', key='root_macaroon')
         else:
-            # let's flag the new one only, not the deprecated one
-            missing = {'caveat_id': [FIELD_REQUIRED]}
-            return errors.INVALID_DATA(**missing)
+            method = auth.refresh_discharge
+            params = (discharge_macaroon_raw,)
+            stats.increment('macaroon.refresh', key='just_discharge')
 
         try:
             new_discharge = method(*params)

=== modified file 'src/api/v20/tests/test_handlers.py'
--- src/api/v20/tests/test_handlers.py	2016-05-05 11:39:43 +0000
+++ src/api/v20/tests/test_handlers.py	2016-05-05 15:40:29 +0000
@@ -2473,8 +2473,7 @@
         self.discharge_macaroon = self.root_macaroon.prepare_for_request(
             unbound_discharge)
 
-        self.data = {'caveat_id': caveat.caveat_id,
-                     'discharge_macaroon': unbound_discharge.serialize()}
+        self.data = {'discharge_macaroon': unbound_discharge.serialize()}
 
     def do_post(self, data=None, expected_status_code=200,
                 content_type='application/json', **kwargs):
@@ -2514,17 +2513,6 @@
         )
         self.assertEqual(self.login_failed_calls, [])
 
-    def test_required_parameters_caveat_id(self):
-        json_body = self.do_post(expected_status_code=400,
-                                 data={'discharge_macaroon': 'whatever'})
-
-        self.assertEqual(json_body, {
-            'code': 'INVALID_DATA',
-            'extra': {'caveat_id': [handlers.FIELD_REQUIRED]},
-            'message': 'Invalid request data'},
-        )
-        self.assertEqual(self.login_failed_calls, [])
-
     def test_account_missing(self):
         self.account.deactivate()
         self.assert_failed_login('ACCOUNT_DEACTIVATED', self.data)
@@ -2557,26 +2545,9 @@
         v.satisfy_general(lambda c: True)
         v.verify(
             self.root_macaroon, self.macaroon_random_key, [discharge_macaroon])
-        self.check_stats_increment('macaroon.refresh', key='macaroon')
-
-    def test_caveat_id_corrupt(self):
-        data = dict(caveat_id="I'm a seriously corrupted caveat",
-                    discharge_macaroon="Also broken")
-        self.assert_failed_login('INVALID_DATA', data,
-                                 expected_status_code=400,
-                                 check_login_failed=False)
-
-    def test_caveat_id_bad_authinfo(self):
-        macaroon, _, _ = self.build_macaroon(service_location="other service")
-        (caveat,) = [c for c in macaroon.third_party_caveats()
-                     if c.location == "other service"]
-        data = dict(caveat_id=caveat.caveat_id,
-                    discharge_macaroon=self.discharge_macaroon.serialize())
-        self.assert_failed_login('INVALID_CREDENTIALS', data,
-                                 expected_status_code=401,
-                                 check_login_failed=False)
-
-    def test_caveat_id_refreshed(self):
+        self.check_stats_increment('macaroon.refresh', key='root_macaroon')
+
+    def test_just_discharge_refreshed(self):
         json_body = self.do_post()
 
         # get new discharge macaroon, apply it and verify with old root (its
@@ -2588,4 +2559,4 @@
         v = Verifier()
         v.satisfy_general(lambda c: True)
         v.verify(self.root_macaroon, self.macaroon_random_key, [discharge])
-        self.check_stats_increment('macaroon.refresh', key='caveat_id')
+        self.check_stats_increment('macaroon.refresh', key='just_discharge')

=== modified file 'src/identityprovider/auth.py'
--- src/identityprovider/auth.py	2016-05-05 11:39:43 +0000
+++ src/identityprovider/auth.py	2016-05-05 15:40:29 +0000
@@ -672,7 +672,7 @@
     return discharge
 
 
-def refresh_discharge(raw_caveat_id, discharge_macaroon_raw):
+def refresh_discharge(discharge_macaroon_raw):
     """Refresh a discharge with a new discharge macaroon."""
     try:
         discharge_macaroon = Macaroon.deserialize(discharge_macaroon_raw)
@@ -680,7 +680,7 @@
         raise ValidationError("The received Macaroons are corrupt")
 
     # get the decrypted deserialized info
-    caveat_info = _decrypt_caveat(raw_caveat_id)
+    caveat_info = _decrypt_caveat(discharge_macaroon.identifier)
     account, macaroon_info = _account_from_macaroon(
         discharge_macaroon, caveat_info['3rdparty'])
 
@@ -689,7 +689,7 @@
     # be matched and verified), and keeping the last_auth and expires from
     # the original discharge macaroon
     discharge = _build_discharge(
-        caveat_info['3rdparty'], raw_caveat_id, account,
+        caveat_info['3rdparty'], discharge_macaroon.identifier, account,
         last_auth=macaroon_info['last_auth'], expires=macaroon_info['expires'])
 
     # return the unbound discharge macaroon

=== modified file 'src/identityprovider/tests/test_auth.py'
--- src/identityprovider/tests/test_auth.py	2016-05-05 11:39:43 +0000
+++ src/identityprovider/tests/test_auth.py	2016-05-05 15:40:29 +0000
@@ -1270,42 +1270,25 @@
         root_macaroon, _, self.random_key = self.build_macaroon()
         (caveat,) = [c for c in root_macaroon.third_party_caveats()
                      if c.location == settings.MACAROON_SERVICE_LOCATION]
-        self.caveat_id = caveat.caveat_id
 
         # get a discharge for the test macaroon
         self.account = self.factory.make_account()
         self.discharge_macaroon = build_discharge_macaroon(
-            self.account, self.caveat_id)
-
-    def test_caveat_id_corrupt(self):
-        self.assertRaises(
-            ValidationError, refresh_discharge,
-            "Corrupted caveat id", self.discharge_macaroon.serialize())
+            self.account, caveat.caveat_id)
 
     def test_discharge_macaroon_corrupt(self):
         self.assertRaises(
-            ValidationError, refresh_discharge,
-            self.caveat_id, "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()
-        (other_caveat,) = [c for c in other_root.third_party_caveats()
-                           if c.location == settings.MACAROON_SERVICE_LOCATION]
-        other_discharge = build_discharge_macaroon(
-            self.account, other_caveat.caveat_id)
-        self.assertRaises(AuthenticationError, refresh_discharge,
-                          self.caveat_id, other_discharge.serialize())
+            ValidationError, refresh_discharge, "Seriously corrupted macaroon")
 
     def test_deactivated_account(self):
         self.account.deactivate()
         self.assertRaises(AccountDeactivated, refresh_discharge,
-                          self.caveat_id, self.discharge_macaroon.serialize())
+                          self.discharge_macaroon.serialize())
 
     def test_password_changed(self):
         self.account.set_password("a new password")
         self.assertRaises(AuthenticationError, refresh_discharge,
-                          self.caveat_id, self.discharge_macaroon.serialize())
+                          self.discharge_macaroon.serialize())
 
     def test_password_with_no_datetime(self):
         # simulate an "old" account password (before we started to keep the
@@ -1317,7 +1300,7 @@
         assert self.account.accountpassword.date_changed is None
 
         # check that auths ok
-        refresh_discharge(self.caveat_id, self.discharge_macaroon.serialize())
+        refresh_discharge(self.discharge_macaroon.serialize())
 
     def test_proper_refreshing(self):
         old_discharge = self.discharge_macaroon  # just rename for readability
@@ -1340,8 +1323,7 @@
 
         # call!
         before = now()
-        new_discharge = refresh_discharge(self.caveat_id,
-                                          old_discharge.serialize())
+        new_discharge = refresh_discharge(old_discharge.serialize())
         after = now()
 
         # test