Merge lp:~facundo/canonical-identity-provider/new-era-discharge into lp:canonical-identity-provider/release

[Facundo Batista]

Accept the caveat_id (instead of macaroon) and return the unbound discharge (instead of binding it) for both create and refresh discharges.

Accept the caveat_id (instead of macaroon) and return the unbound discharge (instead of binding it) for both create and refresh discharges.

[Ricardo Kirkner (ricardokirkner)]

LGTM

review: Approve

[Facundo Batista (facundo)]

review: Approve

[Ubuntu One Auto Pilot (otto-pilot)]

The attempt to merge lp:~facundo/canonical-identity-provider/new-era-discharge into lp:canonical-identity-provider failed. Below is the output from the failed tests.

Bootstrapping...
rm -rf /mnt/tarmac/cache/canonical-identity-provider/merges/trunk/env
rm -rf branches/wheels
rm -rf branches/*
rm -rf staticfiles
rm -f lib/versioninfo.py
find -name '*.pyc' -delete
find -name '*.~*' -delete
Not deleting /mnt/tarmac/cache/canonical-identity-provider/merges/trunk/env/bin
New python executable in /mnt/tarmac/cache/canonical-identity-provider/merges/trunk/env/bin/python
Installing setuptools, pip...done.
/usr/lib/config-manager/cm.py update config-manager.txt
touch branches/last_build
[ -d branches/wheels ] && (cd branches/wheels && bzr pull) || (bzr branch lp:~ubuntuone-pqm-team/canonical-identity-provider/dependencies branches/wheels)
bzr version-info --format=python > lib/versioninfo.py
/mnt/tarmac/cache/canonical-identity-provider/merges/trunk/env/bin/python /mnt/tarmac/cache/canonical-identity-provider/merges/trunk/env/bin/pip install --find-links=branches/wheels --no-index -r requirements.txt
Ignoring indexes: https://pypi.python.org/simple/
Downloading/unpacking bson==0.3.3 (from -r requirements.txt (line 1))
Downloading/unpacking canonical-raven==0.0.3 (from -r requirements.txt (line 2))
Downloading/unpacking convoy==0.4.1 (from -r requirements.txt (line 3))
Downloading/unpacking django==1.8.13 (from -r requirements.txt (line 4))
Downloading/unpacking django-honeypot==0.4.0 (from -r requirements.txt (line 5))
Downloading/unpacking django-jsonfield==0.9.13 (from -r requirements.txt (line 6))
Downloading/unpacking django-model-utils==2.2 (from -r requirements.txt (line 7))
Downloading/unpacking django-modeldict==1.4.1 (from -r requirements.txt (line 8))
Downloading/unpacking django-preflight==0.1.5 (from -r requirements.txt (line 9))
Downloading/unpacking django-secure==1.0.1 (from -r requirements.txt (line 10))
Downloading/unpacking django-statsd-mozilla==0.3.15 (from -r requirements.txt (line 11))
Downloading/unpacking gargoyle==0.11.0 (from -r requirements.txt (line 12))
Downloading/unpacking gunicorn==19.3.0 (from -r requirements.txt (line 13))
Downloading/unpacking lazr.authentication==0.1.3 (from -r requirements.txt (line 14))
Downloading/unpacking libnacl==1.3.6 (from -r requirements.txt (line 15))
Downloading/unpacking nexus==0.3.1 (from -r requirements.txt (line 16))
Downloading/unpacking oath==1.4.0 (from -r requirements.txt (line 17))
Downloading/unpacking oauthlib==0.7.2 (from -r requirements.txt (line 18))
Requirement already satisfied (use --upgrade to upgrade): oops==0.0.13 in /usr/lib/pymodules/python2.7 (from -r requirements.txt (line 19))
Downloading/unpacking oops-amqp==0.0.8b1 (from -r requirements.txt (line 20))
Downloading/unpacking oops-datedir-repo==0.0.23 (from -r requirements.txt (line 21))
Downloading/unpacking oops-dictconfig==0.0.6 (from -r requirements.txt (line 22))
Downloading/unpacking oops-timeline==0.0.2 (from -r requirements.txt (line 23))
Downloading/unpacking oops-wsgi==0.0.11 (from -r requirements.txt (line 24))
Downloading/unpacking paste==2.0.1 (from -r requirements.txt (line 25))
Requirement already s...

[Facundo Batista (facundo)]

Already fixed

review: Approve

[William Grant (wgrant)]

review: Needs Fixing

[Facundo Batista (facundo)]

Addressed the "no caveat id in refreshing" issue

=== modified file 'src/api/v20/handlers.py'
--- src/api/v20/handlers.py	2016-04-18 15:53:18 +0000
+++ src/api/v20/handlers.py	2016-05-05 12:45:29 +0000
@@ -58,6 +58,7 @@
     TokenScope,
 )
 from identityprovider.signals import login_failed, login_succeeded
+from identityprovider.stats import stats
 from identityprovider.timeline_helpers import get_request_timing_function
 from identityprovider.utils import redirection_url_for_token
 
@@ -446,11 +447,19 @@
         if 'macaroon' in data:
             simple_macaroon = True
             root_macaroons_info = [(None, data['macaroon'])]
+            stats.increment('macaroon.discharge', key='single_macaroon')
         elif 'macaroons' in data:
             simple_macaroon = False
             root_macaroons_info = data['macaroons']
+            stats.increment('macaroon.discharge', key='multi_macaroons')
+        elif 'caveat_id' in data:
+            simple_macaroon = True
+            root_macaroons_info = None
+            caveat_id = data['caveat_id']
+            stats.increment('macaroon.discharge', key='caveat_id')
         else:
-            missing = {'macaroon or macaroons': [FIELD_REQUIRED]}
+            # let's flag the new one only, not the deprecated ones
+            missing = {'caveat_id': [FIELD_REQUIRED]}
             return errors.INVALID_DATA(**missing)
 
         account = None
@@ -490,16 +499,22 @@
         if response is not None:
             return response
 
-        all_discharges = []
-        for mac_id, root_macaroon_raw in root_macaroons_info:
-            try:
-                discharge = auth.build_discharge_macaroon(
-                    account, root_macaroon_raw)
-            except ValidationError:
-                return errors.INVALID_DATA()
-            except AuthenticationError:
-                return errors.INVALID_CREDENTIALS()
-            all_discharges.append((mac_id, discharge.serialize()))
+        try:
+            if root_macaroons_info is None:
+                discharge = auth.build_discharge_macaroon(account, caveat_id)
+                all_discharges = [(None, discharge.serialize())]
+            else:
+                # 2016-05-03: this is deprecated, just supporting it until
+                # no more clients send root macaroon(s)
+                all_discharges = []
+                for mac_id, root_macaroon_raw in root_macaroons_info:
+                    discharge = auth.build_discharge_macaroon_from_root(
+                        account, root_macaroon_raw)
+                    all_discharges.append((mac_id, discharge.serialize()))
+        except ValidationError:
+            return errors.INVALID_DATA()
+        except AuthenticationError:
+            return errors.INVALID_CREDENTIALS()
 
         response = rc.ALL_OK
         if simple_macaroon:
@@ -523,16 +538,26 @@
 
         data = request.data
         try:
-            root_macaroon_raw = data['root_macaroon']
             discharge_macaroon_raw = data['discharge_macaroon']
         except KeyError:
-            expected = {'root_macaroon', 'discharge_macaroon'}
-            missing = dict((k, [FIELD_REQUIRED]) for k in expected - set(data))
+            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:
+            method = auth.refresh_macaroons
+            params = (data['root_macaroon'], discharge_macaroon_raw)
+            stats.increment('macaroon.refresh', key='macaroon')
+        else:
+            # let's flag the new one only, not the deprecated one
+            missing = {'caveat_id': [FIELD_REQUIRED]}
             return errors.INVALID_DATA(**missing)
 
         try:
-            new_discharge = auth.refresh_macaroons(root_macaroon_raw,
-                                                   discharge_macaroon_raw)
+            new_discharge = method(*params)
         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-04-18 15:53:18 +0000
+++ src/api/v20/tests/test_handlers.py	2016-05-05 12:45:29 +0000
@@ -2169,13 +2169,25 @@
         self.login_failed_calls = []
         login_failed.connect(self.track_failed_logins, dispatch_uid=self.id())
 
-        self.root_macaroon, self.macaroon_random_key = self.build_macaroon()
+        self.root_macaroon, self.macaroon_random_key, _ = self.build_macaroon()
         self.data = dict(
             email='[e-mail]', password='foobar123',
             macaroon=self.root_macaroon.serialize())
         self.account = self.factory.make_account(
             email=self.data['email'], password=self.data['password'])
 
+        self._stats_increment = self.patch('api.v20.handlers.stats.increment')
+
+    def check_stats_increment(self, *args, **kwargs):
+        """Check stats increment was properly called."""
+        calls = self._stats_increment.call_args_list
+        self.assertEqual(len(calls), 1)
+        call_args, call_kwargs = calls[0]
+        self.assertEqual(call_args, args,
+                         "Bad args in stats increment call {}".format(calls))
+        self.assertEqual(call_kwargs, kwargs,
+                         "Bad kwargs in stats increment call {}".format(calls))
+
 
 class MacaroonDischargeHandlerTestCase(MacaroonHandlerBaseTestCase,
                                        AuthLogTestCaseMixin):
@@ -2199,7 +2211,7 @@
                          response)
         return json.loads(response.content)
 
-    def assert_response_correct(self, json_body, account):
+    def assert_response_correct(self, json_body, account, bind=False):
         """Check we received a good discharge macaroon for the root sent.
 
         Proper verification of the received macaroon internals happens in
@@ -2207,6 +2219,9 @@
         """
         discharge_macaroon_raw = json_body['discharge_macaroon']
         discharge_macaroon = Macaroon.deserialize(discharge_macaroon_raw)
+        if bind:
+            discharge_macaroon = self.root_macaroon.prepare_for_request(
+                discharge_macaroon)
         v = Verifier()
         v.satisfy_general(lambda c: True)
         v.verify(
@@ -2327,6 +2342,7 @@
     def test_macaroon_created(self):
         json_body = self.do_post()
         self.assert_response_correct(json_body, self.account)
+        self.check_stats_increment('macaroon.discharge', key='single_macaroon')
 
     def test_root_macaroon_corrupt(self):
         data = dict(
@@ -2337,23 +2353,23 @@
                                  check_login_failed=False)
 
     def test_root_macaroon_not_for_sso(self):
-        macaroon, _ = self.build_macaroon(service_location="other service")
+        macaroon, _, _ = self.build_macaroon(service_location="other service")
         data = dict(email='[e-mail]', password='foobar123',
                     macaroon=macaroon.serialize())
         self.assert_failed_login('INVALID_CREDENTIALS', data,
                                  expected_status_code=401,
                                  check_login_failed=False)
 
-    def test_missing_macaroons(self):
+    def test_missing_macaroon_info(self):
         data = dict(email='[e-mail]', password='foobar123')
-        extra = {'macaroon or macaroons': [handlers.FIELD_REQUIRED]}
+        extra = {'caveat_id': [handlers.FIELD_REQUIRED]}
         self.assert_failed_login('INVALID_DATA', data, extra=extra,
                                  expected_status_code=400,
                                  check_login_failed=False)
 
     def test_multiple_macaroons(self):
         # build *several* macaroons and send them all at once
-        roots, rkeys = zip(*[self.build_macaroon() for _ in range(3)])
+        roots, rkeys, _ = zip(*[self.build_macaroon() for _ in range(3)])
         ids = range(3)
         roots = dict(zip(ids, roots))
         rkeys = dict(zip(ids, rkeys))
@@ -2372,8 +2388,11 @@
             v.satisfy_general(verifying_function)
             v.verify(root_macaroon, key, [discharge_macaroon])
 
+        self.check_stats_increment('macaroon.discharge', key='multi_macaroons')
+
     def test_multiple_mixed(self):
-        bad_macaroon, _ = self.build_macaroon(service_location="other service")
+        bad_macaroon, _, _ = self.build_macaroon(
+            service_location="other service")
         data = dict(email='[e-mail]', password='foobar123', macaroons=[
             ('good', self.root_macaroon.serialize()),
             ('bad', bad_macaroon.serialize()),
@@ -2382,6 +2401,24 @@
                                  expected_status_code=401,
                                  check_login_failed=False)
 
+    def test_caveat_id_discharge_created(self):
+        # discharge the test macaroon
+        (caveat,) = [c for c in self.root_macaroon.third_party_caveats()
+                     if c.location == settings.MACAROON_SERVICE_LOCATION]
+        data = dict(email='[e-mail]', password='foobar123',
+                    caveat_id=caveat.caveat_id)
+
+        json_body = self.do_post(data=data)
+        self.assert_response_correct(json_body, self.account, bind=True)
+        self.check_stats_increment('macaroon.discharge', key='caveat_id')
+
+    def test_caveat_id_corrupt(self):
+        data = dict(email='[e-mail]', password='foobar123',
+                    caveat_id="I'm a seriously corrupted caveat")
+        self.assert_failed_login('INVALID_DATA', data,
+                                 expected_status_code=400,
+                                 check_login_failed=False)
+
 
 class MacaroonHandlerTimelineTestCase(MacaroonHandlerBaseTestCase,
                                       TimelineActionMixin):
@@ -2428,12 +2465,16 @@
         super(MacaroonRefreshHandlerTestCase, self).setUp()
 
         # discharge the test macaroon
-        root_macaroon_raw = self.root_macaroon.serialize()
-        self.discharge_macaroon = build_discharge_macaroon(
-            self.account, root_macaroon_raw)
-
-        self.data = {'root_macaroon': root_macaroon_raw,
-                     'discharge_macaroon': self.discharge_macaroon.serialize()}
+        (caveat,) = [c for c in self.root_macaroon.third_party_caveats()
+                     if c.location == settings.MACAROON_SERVICE_LOCATION]
+
+        unbound_discharge = build_discharge_macaroon(
+            self.account, caveat.caveat_id)
+        self.discharge_macaroon = self.root_macaroon.prepare_for_request(
+            unbound_discharge)
+
+        self.data = {'caveat_id': caveat.caveat_id,
+                     'discharge_macaroon': unbound_discharge.serialize()}
 
     def do_post(self, data=None, expected_status_code=200,
                 content_type='application/json', **kwargs):
@@ -2463,13 +2504,23 @@
         self.assertEqual(json_body['extra'], extra)
         return json_body
 
-    def test_required_parameters(self):
+    def test_required_parameters_discharge(self):
         json_body = self.do_post(expected_status_code=400, data={})
 
         self.assertEqual(json_body, {
             'code': 'INVALID_DATA',
-            'extra': {'root_macaroon': [handlers.FIELD_REQUIRED],
-                      'discharge_macaroon': [handlers.FIELD_REQUIRED]},
+            'extra': {'discharge_macaroon': [handlers.FIELD_REQUIRED]},
+            'message': 'Invalid request data'},
+        )
+        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, [])
@@ -2486,7 +2537,7 @@
                                  check_login_failed=False)
 
     def test_macaroon_bad_authinfo(self):
-        macaroon, _ = self.build_macaroon(service_location="other service")
+        macaroon, _, _ = self.build_macaroon(service_location="other service")
         data = dict(root_macaroon=macaroon.serialize(),
                     discharge_macaroon=self.discharge_macaroon.serialize())
         self.assert_failed_login('INVALID_CREDENTIALS', data,
@@ -2494,7 +2545,9 @@
                                  check_login_failed=False)
 
     def test_macaroon_refreshed(self):
-        json_body = self.do_post()
+        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)
@@ -2504,3 +2557,35 @@
         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):
+        json_body = self.do_post()
+
+        # get new discharge macaroon, apply it and verify with old root (its
+        # internals are verified by the tests of the 'refresh_macaroons'
+        # function itself)
+        unbound_discharge_raw = json_body['discharge_macaroon']
+        unbound_discharge = Macaroon.deserialize(unbound_discharge_raw)
+        discharge = self.root_macaroon.prepare_for_request(unbound_discharge)
+        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')
 
=== modified file 'src/identityprovider/auth.py'
--- src/identityprovider/auth.py	2016-03-23 16:20:24 +0000
+++ src/identityprovider/auth.py	2016-05-05 12:45:29 +0000
@@ -507,16 +507,21 @@
         # the macaroon doesn't have a location for this service
         raise AuthenticationError("The received macaroon is not for ours")
 
-    # get the only-for-this-project random key
+    caveat_info = _decrypt_caveat(sso_caveat.caveat_id)
+    return sso_caveat.caveat_id, caveat_info
+
+
+def _decrypt_caveat(raw_caveat_id):
+    """Decrypt and decode the caveat id info."""
     try:
         caveat_info_raw = settings.CRYPTO_SSO_PRIVKEY.decrypt(
-            base64.b64decode(sso_caveat.caveat_id))
+            base64.b64decode(raw_caveat_id))
         caveat_info = json.loads(caveat_info_raw)
     except:
         # not properly encrypted information inside
-        raise AuthenticationError("Bad info in the caveat_id")
+        raise ValidationError("Bad info in the caveat_id")
 
-    return sso_caveat.caveat_id, caveat_info
+    return caveat_info
 
 
 def _build_discharge(macaroon_key, caveat_id, account,
@@ -556,8 +561,12 @@
     return d
 
 
-def build_discharge_macaroon(account, root_macaroon_raw):
-    """Build a discharge macaroon from a root one."""
+def build_discharge_macaroon_from_root(account, root_macaroon_raw):
+    """Build a discharge macaroon from a root one.
+
+    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)
     except:
@@ -576,18 +585,24 @@
     return discharge
 
 
-def refresh_macaroons(root_macaroon_raw, discharge_macaroon_raw):
-    """Refresh a root/discharge pair with a new discharge macaroon."""
+def build_discharge_macaroon(account, raw_caveat_id):
+    """Build a discharge macaroon from the caveat id."""
+    # get the decrypted deserialized info
+    caveat_info = _decrypt_caveat(raw_caveat_id)
+
+    # create a discharge macaroon with same location, key and
+    # identifier than it's original 3rd-party caveat (so they can
+    # be matched and verified)
+    discharge = _build_discharge(
+        caveat_info['3rdparty'], raw_caveat_id, account)
+
+    # return the unbound discharge macaroon
+    return discharge
+
+
+def _account_from_macaroon(macaroon, key, discharge=None):
+    """Get the new discharge macaroon."""
     service_location = settings.MACAROON_SERVICE_LOCATION
-
-    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)
     info_holder = {}
 
     def checker(caveat):
@@ -606,7 +621,10 @@
     v = Verifier()
     v.satisfy_general(checker)
     try:
-        v.verify(root_macaroon, caveat_info['roothash'], [discharge_macaroon])
+        if discharge is None:
+            v.verify(macaroon, key)
+        else:
+            v.verify(macaroon, key, [discharge])
     except:
         raise AuthenticationError("Not verifying macaroons")
 
@@ -620,14 +638,59 @@
         if account.accountpassword.date_changed > last_auth_ts:
             raise AuthenticationError("Password changed")
 
+    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=info_holder['last_auth'],
-                         expires=info_holder['expires'])
+    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(raw_caveat_id, discharge_macaroon_raw):
+    """Refresh a discharge with a new discharge macaroon."""
+    try:
+        discharge_macaroon = Macaroon.deserialize(discharge_macaroon_raw)
+    except:
+        raise ValidationError("The received Macaroons are corrupt")
+
+    # get the decrypted deserialized info
+    caveat_info = _decrypt_caveat(raw_caveat_id)
+    account, macaroon_info = _account_from_macaroon(
+        discharge_macaroon, caveat_info['3rdparty'])
+
+    # 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
+    discharge = _build_discharge(
+        caveat_info['3rdparty'], raw_caveat_id, account,
+        last_auth=macaroon_info['last_auth'], expires=macaroon_info['expires'])
+
+    # return the unbound discharge macaroon
+    return discharge
 
=== modified file 'src/identityprovider/tests/test_auth.py'
--- src/identityprovider/tests/test_auth.py	2016-04-01 19:35:14 +0000
+++ src/identityprovider/tests/test_auth.py	2016-05-05 12:45:29 +0000
@@ -29,9 +29,11 @@
     LaunchpadBackend,
     SSOOAuthAuthentication,
     SSORequestValidator,
-    _get_own_caveat,
+    _decrypt_caveat,
     basic_authenticate,
     build_discharge_macaroon,
+    build_discharge_macaroon_from_root,
+    refresh_discharge,
     refresh_macaroons,
     validate_oauth_signature,
 )
@@ -958,26 +960,31 @@
             timer=self.dummy_timer)
 
 
-class BuildMacaroonDischargeTestCase(SSOBaseTestCase):
+class BuildMacaroonFromRootDischargeTestCase(SSOBaseTestCase):
+    """Test the deprecated build_discharge_macaroon_from_root.
+
+    These tests are kept separated as that function won't evolve no more.
+    """
 
     def setUp(self):
-        super(BuildMacaroonDischargeTestCase, self).setUp()
-        self.root_macaroon, self.macaroon_random_key = self.build_macaroon()
+        super(BuildMacaroonFromRootDischargeTestCase, self).setUp()
+        self.root_macaroon, self.macaroon_random_key, _ = self.build_macaroon()
 
     def test_root_macaroon_corrupt(self):
-        self.assertRaises(ValidationError, build_discharge_macaroon,
+        self.assertRaises(ValidationError, build_discharge_macaroon_from_root,
                           "fake account", "I'm a seriously corrupted macaroon")
 
     def test_root_macaroon_not_for_sso(self):
-        macaroon, _ = self.build_macaroon(service_location="other service")
-        self.assertRaises(AuthenticationError, build_discharge_macaroon,
+        macaroon, _, _ = self.build_macaroon(service_location="other service")
+        self.assertRaises(AuthenticationError,
+                          build_discharge_macaroon_from_root,
                           "fake account", macaroon.serialize())
 
     def test_proper_discharging(self):
         # build the input and call
         real_account = self.factory.make_account()
         before = now()
-        discharge_macaroon = build_discharge_macaroon(
+        discharge_macaroon = build_discharge_macaroon_from_root(
             real_account, self.root_macaroon.serialize())
         after = now()
 
@@ -1029,6 +1036,76 @@
                  [discharge_macaroon])
 
 
+class BuildMacaroonDischargeTestCase(SSOBaseTestCase):
+
+    def test_caveat_id_corrupt(self):
+        self.assertRaises(ValidationError, build_discharge_macaroon,
+                          "fake account", "I'm a seriously corrupted caveatid")
+
+    def test_caveat_id_not_for_sso(self):
+        macaroon, _, _ = self.build_macaroon(service_location="other service")
+        (sso_caveat,) = [c for c in macaroon.third_party_caveats()
+                         if c.location == "other service"]
+
+        self.assertRaises(ValidationError, build_discharge_macaroon,
+                          "fake account", sso_caveat)
+
+    def test_proper_discharging(self):
+        # build the input and call
+        root_macaroon, _, random_key = self.build_macaroon()
+        (sso_caveat,) = [c for c in root_macaroon.third_party_caveats()
+                         if c.location == settings.MACAROON_SERVICE_LOCATION]
+
+        real_account = self.factory.make_account()
+        before = now()
+        discharge_macaroon = build_discharge_macaroon(
+            real_account, sso_caveat.caveat_id)
+        after = now()
+
+        # test
+        def checker(caveat):
+            """Assure all caveats inside the discharged macaroon are ok."""
+            source, key, value = caveat.split("|", 2)
+
+            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':
+                last_auth = datetime.strptime(value, '%Y-%m-%dT%H:%M:%S.%f')
+                self.assertGreater(last_auth, before)
+                self.assertGreater(after, last_auth)
+                return True
+
+            if key == 'account':
+                acc = json.loads(base64.b64decode(value).decode("utf8"))
+                self.assertEqual(acc['openid'], real_account.openid_identifier)
+                self.assertEqual(acc['email'],
+                                 real_account.preferredemail.email)
+                self.assertEqual(acc['displayname'], real_account.displayname)
+                self.assertEqual(acc['is_verified'], real_account.is_verified)
+                return True
+
+            if key == 'expires':
+                expires = datetime.strptime(value, '%Y-%m-%dT%H:%M:%S.%f')
+                before_plus_ttl = before + timedelta(
+                    seconds=settings.MACAROON_TTL)
+                after_plus_ttl = after + timedelta(
+                    seconds=settings.MACAROON_TTL)
+                self.assertGreater(expires, before_plus_ttl)
+                self.assertGreater(after_plus_ttl, expires)
+                return True
+
+            # we're not validating an SSO from the discharged macaroon, fail!
+            return False
+
+        v = Verifier()
+        v.satisfy_general(checker)
+        v.verify(discharge_macaroon, random_key, [])
+
+
 class MacaroonHelpersTestCase(SSOBaseTestCase):
 
     def test_get_caveat_ok(self):
@@ -1052,42 +1129,29 @@
             settings.MACAROON_SERVICE_LOCATION, random_key, info_encrypted)
 
         # check
-        raw_caveat_id, caveat_info = _get_own_caveat(root_macaroon)
-        self.assertEqual(raw_caveat_id, info_encrypted)
+        (sso_caveat,) = [c for c in root_macaroon.third_party_caveats()
+                         if c.location == settings.MACAROON_SERVICE_LOCATION]
+        caveat_info = _decrypt_caveat(sso_caveat.caveat_id)
         self.assertEqual(caveat_info, info)
 
-    def test_get_caveat_not_for_sso(self):
-        macaroon, _ = self.build_macaroon(service_location="other service")
-        self.assertRaises(AuthenticationError, _get_own_caveat, macaroon)
-
     def test_get_caveat_badly_encrypted(self):
-        test_rsa_priv_key, test_rsa_pub_key = self.setup_key_pair()
-
-        # create a Macaron with the proper third party caveat
-        macaroon_random_key = binascii.hexlify(os.urandom(32))
-        root_macaroon = Macaroon(
-            location='The store ;)',
-            key=macaroon_random_key,
-            identifier='A test macaroon',
-        )
-        random_key = binascii.hexlify(os.urandom(32))
-        root_macaroon.add_third_party_caveat(
-            settings.MACAROON_SERVICE_LOCATION, random_key,
-            b"not really well encrypted stuff")
-
-        # check
-        self.assertRaises(AuthenticationError, _get_own_caveat, root_macaroon)
-
-
-class MacaroonRefreshTestCase(SSOBaseTestCase):
+        self.assertRaises(ValidationError,
+                          _decrypt_caveat, b"not really well encrypted stuff")
+
+
+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(MacaroonRefreshTestCase, self).setUp()
-        self.root_macaroon, self.macaroon_random_key = self.build_macaroon()
+        super(MacaroonRefreshFromRootTestCase, self).setUp()
+        self.root_macaroon, self.macaroon_random_key, _ = self.build_macaroon()
 
         # discharge the test macaroon
         self.account = self.factory.make_account()
-        self.discharge_macaroon = build_discharge_macaroon(
+        self.discharge_macaroon = build_discharge_macaroon_from_root(
             self.account, self.root_macaroon.serialize())
 
     def test_root_macaroon_corrupt(self):
@@ -1102,8 +1166,8 @@
 
     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_discharge = build_discharge_macaroon(
+        other_root, _, _ = self.build_macaroon()
+        other_discharge = build_discharge_macaroon_from_root(
             self.account, other_root.serialize())
         self.assertRaises(AuthenticationError, refresh_macaroons,
                           self.root_macaroon.serialize(),
@@ -1197,3 +1261,120 @@
         v.satisfy_general(checker)
         v.verify(self.root_macaroon, self.macaroon_random_key,
                  [new_discharge])
+
+
+class MacaroonRefreshTestCase(SSOBaseTestCase):
+
+    def setUp(self):
+        super(MacaroonRefreshTestCase, self).setUp()
+        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())
+
+    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())
+
+    def test_deactivated_account(self):
+        self.account.deactivate()
+        self.assertRaises(AccountDeactivated, refresh_discharge,
+                          self.caveat_id, 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())
+
+    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_discharge(self.caveat_id, 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_discharge(self.caveat_id,
+                                          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 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':
+                acc = json.loads(base64.b64decode(value).decode("utf8"))
+                self.assertEqual(acc['openid'], self.account.openid_identifier)
+                self.assertEqual(acc['email'], new_mail.email)
+                self.assertEqual(acc['displayname'], "New test display name")
+                self.assertEqual(acc['is_verified'], self.account.is_verified)
+                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(new_discharge, self.random_key, [])
 
=== modified file 'src/identityprovider/tests/test_forms.py'
--- src/identityprovider/tests/test_forms.py	2016-04-11 11:57:38 +0000
+++ src/identityprovider/tests/test_forms.py	2016-05-05 12:45:29 +0000
@@ -1138,7 +1138,7 @@
         """The server always returns discharge macaroons to trusted sites,
         regardless of the state of the checkbox in the UI.
         """
-        root_macaroon, _ = self.build_macaroon()
+        root_macaroon, _, _ = self.build_macaroon()
         macaroon_request = MacaroonRequest(root_macaroon.serialize())
         form = MacaroonRequestForm(
             request=self._get_request_with_post_args(),
@@ -1149,7 +1149,7 @@
         """The server returns discharge macaroons to untrusted sites when
         the user checks the checkbox in the UI.
         """
-        root_macaroon, _ = self.build_macaroon()
+        root_macaroon, _, _ = self.build_macaroon()
         macaroon_request = MacaroonRequest(root_macaroon.serialize())
         form = MacaroonRequestForm(
             request=self._get_request_with_post_args(macaroon='macaroon'),
@@ -1160,7 +1160,7 @@
         """The server does not return discharge macaroons to untrusted sites
         when the user does not check the checkbox in the UI.
         """
-        root_macaroon, _ = self.build_macaroon()
+        root_macaroon, _, _ = self.build_macaroon()
         macaroon_request = MacaroonRequest(root_macaroon.serialize())
         form = MacaroonRequestForm(
             request=self._get_request_with_post_args(),
@@ -1169,7 +1169,7 @@
 
     def test_checkbox_status_for_trusted_site(self):
         """Checkboxes are always checked if the site is trusted."""
-        root_macaroon, _ = self.build_macaroon()
+        root_macaroon, _, _ = self.build_macaroon()
         macaroon_request = MacaroonRequest(root_macaroon.serialize())
         form = MacaroonRequestForm(
             request=self._get_request_with_post_args(),
@@ -1178,7 +1178,7 @@
 
     def test_checkbox_status_for_untrusted_site(self):
         """Checkboxes are checked by default if the site is untrusted."""
-        root_macaroon, _ = self.build_macaroon()
+        root_macaroon, _, _ = self.build_macaroon()
         macaroon_request = MacaroonRequest(root_macaroon.serialize())
         form = MacaroonRequestForm(
             request=self._get_request_with_post_args(),
@@ -1189,7 +1189,7 @@
         """Checkboxes respect user preferences on untrusted sites where
         available.
         """
-        root_macaroon, _ = self.build_macaroon()
+        root_macaroon, _, _ = self.build_macaroon()
         macaroon_request = MacaroonRequest(root_macaroon.serialize())
         approved_data = {
             'requested': ['macaroon'],
 
=== modified file 'src/identityprovider/tests/test_macaroon.py'
--- src/identityprovider/tests/test_macaroon.py	2016-04-05 22:12:47 +0000
+++ src/identityprovider/tests/test_macaroon.py	2016-05-05 12:45:29 +0000
@@ -45,7 +45,7 @@
     def setUp(self):
         super(MacaroonRequestTestCase, self).setUp()
 
-        self.root_macaroon, self.macaroon_random_key = self.build_macaroon()
+        self.root_macaroon, self.macaroon_random_key, _ = self.build_macaroon()
         self.req = MacaroonRequest(self.root_macaroon.serialize())
 
     def assertMacaroonsEqual(self, expected, observed):
 
=== modified file 'src/identityprovider/tests/test_views_server.py'
--- src/identityprovider/tests/test_views_server.py	2016-04-04 15:41:06 +0000
+++ src/identityprovider/tests/test_views_server.py	2016-05-05 12:45:29 +0000
@@ -227,7 +227,7 @@
         self._test_auto_auth(sreg=['fullname'])
 
     def test_handle_user_response_auto_auth_discharge_macaroon(self):
-        root_macaroon, macaroon_random_key = self.build_macaroon()
+        root_macaroon, macaroon_random_key, _ = self.build_macaroon()
         # Add padding to force a POST after signing.  We don't know exactly
         # how long the serialized discharge macaroon will be yet, but it
         # will probably be at least 1024 bytes.
@@ -1065,7 +1065,7 @@
         # make sure rpconfig is set to auto authorize
         OpenIDRPConfig.objects.create(
             trust_root='http://localhost/', auto_authorize=True)
-        root_macaroon, macaroon_random_key = self.build_macaroon()
+        root_macaroon, macaroon_random_key, _ = self.build_macaroon()
         param_overrides = {
             'openid.ns.macaroon': MACAROON_NS,
             'openid.macaroon.root': root_macaroon.serialize(),
@@ -1089,7 +1089,7 @@
             root_macaroon, macaroon_random_key, [discharge_macaroon]))
 
     def test_state_of_checkboxes_and_data_formats_macaroon(self):
-        root_macaroon, _ = self.build_macaroon()
+        root_macaroon, _, _ = self.build_macaroon()
         param_overrides = {
             'openid.ns.macaroon': MACAROON_NS,
             'openid.macaroon.root': root_macaroon.serialize(),
@@ -1864,7 +1864,7 @@
         if with_teams:
             params['openid.lp.query_membership'] = 'ubuntu-team'
         if with_macaroon:
-            root_macaroon, _ = self.build_macaroon()
+            root_macaroon, _, _ = self.build_macaroon()
             params['openid.ns.macaroon'] = MACAROON_NS
             params['openid.macaroon.root'] = root_macaroon.serialize()
         provider_url = get_provider_url(request)
 
=== modified file 'src/identityprovider/tests/utils.py'
--- src/identityprovider/tests/utils.py	2016-04-28 13:01:53 +0000
+++ src/identityprovider/tests/utils.py	2016-05-05 12:45:29 +0000
@@ -202,7 +202,7 @@
             test_rsa_pub_key.encrypt(json.dumps(info), 32)[0])
         root_macaroon.add_third_party_caveat(
             service_location, random_key, info_encrypted)
-        return root_macaroon, macaroon_random_key
+        return root_macaroon, macaroon_random_key, random_key
 
 
 class SSOBaseTestCase(SSOBaseTestCaseMixin, TestCase):
 
=== modified file 'src/identityprovider/views/server.py'
--- src/identityprovider/views/server.py	2016-04-29 14:17:15 +0000
+++ src/identityprovider/views/server.py	2016-05-05 12:45:29 +0000
@@ -42,7 +42,7 @@
 from openid.yadis.constants import YADIS_HEADER_NAME
 
 from identityprovider import signed
-from identityprovider.auth import build_discharge_macaroon
+from identityprovider.auth import build_discharge_macaroon_from_root
 from identityprovider.const import (
     AX_DATA_FIELDS,
     MACAROON_NS,
@@ -769,7 +769,7 @@
     rpconfig = utils.get_rpconfig(openid_request.trust_root)
     form = MacaroonRequestForm(request, macaroon_request, rpconfig)
     if form.data_approved_for_request:
-        discharge_macaroon = build_discharge_macaroon(
+        discharge_macaroon = build_discharge_macaroon_from_root(
             request.user, macaroon_request.root_macaroon_raw)
         macaroon_response = MacaroonResponse.extractResponse(
             macaroon_request, discharge_macaroon.serialize())