Merge lp:~facundo/canonical-identity-provider/refuse-refresh-password-changed into lp:canonical-identity-provider/release

[Facundo Batista]

Check if account's password changed before refreshing the macaroon.

Check if account's password changed before refreshing the macaroon.

[Daniel Manrique (roadmr)]

The test, I think, is not really doing what we expect it to do. See longish comment below and I think we have some fixing to do :(

review: Needs Fixing

[Facundo Batista (facundo)]

Answered a comment, pushed the fix for the other.

[Daniel Manrique (roadmr)]

Looks OK to me, Matías suggested another way to achieve the None value using update(), your choice on which one to use.

I'd still prefer a more explicit check than "just run this and ensure it didn't exception out" but am OK if you think this is reasonable.

I'll +1 in case you want to merge as is, or if you want to apply the update() change.

review: Approve

[Daniel Manrique (roadmr)]

Thanks for the assert.

review: Approve

[Matias Bordese (matiasb)]

LGTM

review: Approve

=== modified file 'src/identityprovider/auth.py'
--- src/identityprovider/auth.py	2016-03-22 14:04:53 +0000
+++ src/identityprovider/auth.py	2016-03-23 18:07:17 +0000
@@ -46,6 +46,8 @@
 
 logger = logging.getLogger(__name__)
 
+MACAROON_TIMESTAMP_FORMAT = '%Y-%m-%dT%H:%M:%S.%f'
+
 
 class BCrypt13PasswordHasher(BCryptPasswordHasher):
     rounds = 13
@@ -541,12 +543,12 @@
     d.add_first_party_caveat(service_location + '|account|' + account_info)
 
     # add timestamp values
-    now_string = now().strftime('%Y-%m-%dT%H:%M:%S.%f')
+    now_string = now().strftime(MACAROON_TIMESTAMP_FORMAT)
     if last_auth is None:
         last_auth = now_string
     if expires is None:
         expires_ts = now() + datetime.timedelta(seconds=settings.MACAROON_TTL)
-        expires = expires_ts.strftime('%Y-%m-%dT%H:%M:%S.%f')
+        expires = expires_ts.strftime(MACAROON_TIMESTAMP_FORMAT)
 
     d.add_first_party_caveat(service_location + '|valid_since|' + now_string)
     d.add_first_party_caveat(service_location + '|last_auth|' + last_auth)
@@ -608,10 +610,15 @@
     except:
         raise AuthenticationError("Not verifying macaroons")
 
-    # verify that the account is still fine
+    # verify that the account is still active and password didn't change
     account = Account.objects.active_by_openid(info_holder['openid'])
     if account is None:
         raise AccountDeactivated()
+    if account.accountpassword.date_changed is not None:
+        last_auth_ts = datetime.datetime.strptime(info_holder['last_auth'],
+                                                  MACAROON_TIMESTAMP_FORMAT)
+        if account.accountpassword.date_changed > last_auth_ts:
+            raise AuthenticationError("Password changed")
 
     # create the new discharge macaroon with same location, key and
     # identifier than it's original 3rd-party caveat (so they can

=== modified file 'src/identityprovider/tests/test_auth.py'
--- src/identityprovider/tests/test_auth.py	2016-03-22 19:47:29 +0000
+++ src/identityprovider/tests/test_auth.py	2016-03-23 18:07:17 +0000
@@ -1161,6 +1161,25 @@
                           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