Merge lp:~maxiberta/canonical-identity-provider/openid-session-ttl into lp:canonical-identity-provider/release

[Maximiliano Bertacchini]

Expire old OpenID tokens from user sessions.

Tracks openid token expiration in each session (session['token_expirations'], default 24h since creation), and deletes old ones on every request to /+openid. Old tokens are assigned a 24h ttl when first seen.

[Daniel Manrique (roadmr)]

LGTM, my only nitpick is that 24 * 60 * 60 is harder for me to read than 86400, a well-known constant for "seconds in a day". But I acknowledge the 24 * 60 * 60 variant is widely used in this codebase, so let's keep it for consistency.

review: Approve

[Daniel Manrique (roadmr)]

Thanks!

review: Approve

=== modified file 'django_project/settings_base.py'
--- django_project/settings_base.py	2018-07-12 16:40:37 +0000
+++ django_project/settings_base.py	2018-09-05 14:18:47 +0000
@@ -442,6 +442,7 @@
 OPENID_LAUNCHPAD_TEAMS_REQUIRED = []
 OPENID_PHYSICAL_MULTIFACTOR_REQUIRED = False
 OPENID_PREAUTHORIZATION_ACL = []
+OPENID_TOKEN_TTL = 24 * 60 * 60  # 1 day, seconds
 OPENID_SREG_EXTRA_FIELDS = ['language']
 OPENID_SREG_REQUIRED_FIELDS = []
 OPENID_SSO_SERVER_URL = SSO_ROOT_URL
 
=== modified file 'src/identityprovider/tests/test_views_server.py'
--- src/identityprovider/tests/test_views_server.py	2018-05-28 19:44:56 +0000
+++ src/identityprovider/tests/test_views_server.py	2018-09-05 14:18:47 +0000
@@ -1,9 +1,10 @@
 # -*- coding: utf-8 -*-
-# Copyright 2010-2016 Canonical Ltd.  This software is licensed under
+# Copyright 2010-2018 Canonical Ltd.  This software is licensed under
 # the GNU Affero General Public License version 3 (see the file
 # LICENSE).
 
 import datetime
+import time
 import urlparse
 
 from urllib import quote, quote_plus, urlencode
@@ -181,6 +182,7 @@
                          quote_plus(self.account.displayname))
         self.assertEqual(query['openid.ax.value.account_verified.1'],
                          'token_via_email')
+        self.assertIn('token_expirations', r.client.session)
 
     def test_handle_user_response_auto_auth_large_ax_sreg_response(self):
         # Make sure we get a large response
@@ -252,6 +254,7 @@
             self.account.get_absolute_url()))
         self.assertEqual(query['openid.identity'], openid_identity_url)
         self.assertEqual(query['openid.claimed_id'], openid_identity_url)
+        self.assertIn('token_expirations', response.client.session)
 
     def _test_auto_auth(self, ax=None, sreg=None,
                         root_macaroon=None, macaroon_key=None):
@@ -342,6 +345,7 @@
             self.assertNotIn(k, forms[0].fields)
         for k in ('openid.assoc_handle', 'openid.sig'):
             self.assertIn(k, forms[0].fields)
+        self.assertIn('token_expirations', response.client.session)
 
     def test_handle_user_response_openid_is_authorized_idselect(self):
         # update rp to auto authorize
@@ -357,6 +361,7 @@
             self.account.get_absolute_url())
         self.assertEqual(query['openid.identity'],
                          quote_plus(openid_identity_url))
+        self.assertIn('token_expirations', r.client.session)
 
     def test_handle_user_response_openid_is_authorized_other_id(self):
         self.rpconfig.auto_authorize = True
@@ -370,6 +375,7 @@
         self.assertEqual(query['openid.mode'], 'id_res')
         self.assertEqual(query['openid.identity'],
                          quote_plus(self.params['openid.identity']))
+        self.assertIn('token_expirations', r.client.session)
 
     def test_handle_user_response_user_is_authenticated(self):
         url = self.request.build_absolute_uri(
@@ -380,12 +386,19 @@
         query = self.get_query(r)
         self.assertEqual(r.status_code, 302)
         self.assertEqual(query['openid.mode'], 'cancel')
+        self.assertIn('token_expirations', r.client.session)
 
     def test_handle_user_response_decide(self):
         r = self.client.get(self.url, self.params)
         self.assertEqual(r.status_code, 302)
         self.assertTrue(r['Location'].endswith('+decide'))
 
+        session = r.client.session
+        self.assertIn('token_expirations', session)
+        self.assertEqual(len(session['token_expirations']), 1)
+        self.assertIn(session['token_expirations'].keys()[0], session)
+        self.assertIn('token_expirations', r.client.session)
+
     def test_handle_user_response_with_referer(self):
         META = {'HTTP_REFERER': 'http://localhost/'}
         r = self.client.get(self.url, self.params, **META)
@@ -393,6 +406,7 @@
         self.assertEqual(r.status_code, 302)
         self.assertTrue(r['Location'].endswith('+decide'))
         self.assertEqual(openid_referer.value, META['HTTP_REFERER'])
+        self.assertIn('token_expirations', r.client.session)
 
     def get_login_after_redirect_from_consumer(self):
         self.rpconfig.logo = 'http://someserver/logo.png'
@@ -423,6 +437,7 @@
         r = self.client.post(login_url)
         # rpconfig should be refetched
         self.assertEqual(r.context['rpconfig'], self.rpconfig)
+        self.assertIn('token_expirations', r.client.session)
 
 
 class HandleUserResponseUnverifiedUserLogedInTestCase(SSOBaseTestCase):
@@ -627,6 +642,98 @@
         self.assertFalse(valid)
 
 
+class ExpiredTokensHandlerTestCase(SSOBaseTestCase):
+
+    def _generate_openid_token(self, session, token=None, expiration=None):
+        if token is None:
+            token = generate_random_string(16)
+        session[token] = signed.dumps('orequest', settings.SECRET_KEY)
+        if expiration:
+            if 'token_expirations' not in session:
+                session['token_expirations'] = {}
+            session['token_expirations'][token] = expiration
+        return token
+
+    def test_session_with_no_token_expirations(self):
+        session = self.client.session
+        self.assertNotIn('token_expirations', session)
+
+        server.handle_expired_openid_tokens(session)
+
+        self.assertIn('token_expirations', session)
+
+    def test_token_with_no_expiration(self):
+        session = self.client.session
+        token = self._generate_openid_token(session)
+        self.assertIn(token, session)
+        self.assertNotIn('token_expirations', session)
+
+        server.handle_expired_openid_tokens(session)
+
+        self.assertIn(token, session['token_expirations'])
+
+    def test_unexpired_token(self):
+        session = self.client.session
+        expiration = int(time.time()) + 60  # 1m into the future
+        token = self._generate_openid_token(session, expiration=expiration)
+        self.assertIn(token, session)
+        self.assertEqual(session['token_expirations'][token], expiration)
+
+        # Simulate an old session.
+        session.modified = False
+        server.handle_expired_openid_tokens(session)
+
+        self.assertIn(token, session)
+        self.assertIn(token, session['token_expirations'])
+        self.assertEqual(session['token_expirations'][token], expiration)
+        self.assertTrue(session.modified)
+
+    def test_expired_token(self):
+        session = self.client.session
+        expiration = int(time.time()) - 1  # 1s ago
+        token = self._generate_openid_token(session, expiration=expiration)
+        self.assertIn(token, session)
+        self.assertEqual(session['token_expirations'][token], expiration)
+
+        # Simulate an old session.
+        session.modified = False
+        server.handle_expired_openid_tokens(session)
+
+        self.assertNotIn(token, session)
+        self.assertNotIn(token, session['token_expirations'])
+        self.assertTrue(session.modified)
+
+    def test_non_token_keys(self):
+        session = self.client.session
+        for i in range(1, 33):
+            token = 'a' * i
+            self._generate_openid_token(session, token=token)
+
+        server.handle_expired_openid_tokens(session)
+
+        # Only the token with len 16 should be included in 'token_expirations'.
+        self.assertIn('token_expirations', session)
+        self.assertEqual(len(session['token_expirations']), 1)
+        self.assertEqual(len(session['token_expirations'].keys()[0]), 16)
+
+    def test_non_token_values(self):
+        session = self.client.session
+        for i, value in enumerate([
+                None,
+                '',
+                [],
+                {},
+                'a' * 50,  # length of valid signed token
+                ['asdf'],
+                {'a': 1, 'b': 2}
+                ]):
+            session[str(i) * 16] = value
+
+        server.handle_expired_openid_tokens(session)
+        self.assertIn('token_expirations', session)
+        self.assertFalse(session['token_expirations'])
+
+
 class DecideBaseTestCase(AuthenticatedTestCase):
 
     def setUp(self):
@@ -749,6 +856,11 @@
         self.assertEqual(r.status_code, 200)
         self.assertTemplateUsed(r, 'registration/login.html')
 
+        session = r.client.session
+        self.assertIn('token_expirations', session)
+        self.assertEqual(len(session['token_expirations']), 1)
+        self.assertIn(session['token_expirations'].keys()[0], session)
+
     def test_decide_multiple_openidrpsummary(self):
         # create multiple matching OpenIDRPSummary objects
         trust_root = 'http://localhost/'
@@ -1923,6 +2035,23 @@
         self.assertEqual(r.status_code, 200)
         self.assertTemplateUsed(r, 'server/untrusted.html')
 
+    def test_token_expiration(self):
+        params = {'openid.mode': 'checkid_setup',
+                  'openid.trust_root': 'http://localhost/',
+                  'openid.return_to': 'http://localhost/',
+                  'openid.identity': IDENTIFIER_SELECT}
+        request = self.factory.make_request(**params)
+        provider_url = get_provider_url(request)
+
+        self.assertNotIn('token_expirations', self.client.session)
+
+        response = self.client.get(provider_url, params)
+
+        session = response.client.session
+        self.assertIn('token_expirations', session)
+        self.assertEqual(len(session['token_expirations']), 1)
+        self.assertIn(session['token_expirations'].keys()[0], session)
+
 
 class MarkupTestCase(AuthenticatedTestCase):
 
 
=== modified file 'src/identityprovider/views/server.py'
--- src/identityprovider/views/server.py	2018-05-28 19:44:56 +0000
+++ src/identityprovider/views/server.py	2018-09-05 14:18:47 +0000
@@ -1,9 +1,10 @@
-# Copyright 2010-2016 Canonical Ltd.  This software is licensed under
+# Copyright 2010-2018 Canonical Ltd.  This software is licensed under
 # the GNU Affero General Public License version 3 (see the file
 # LICENSE).
 
 import logging
 import re
+import time
 import urllib
 import urlparse
 from datetime import timedelta
@@ -140,6 +141,9 @@
     except ProtocolError as e:
         response = _handle_openid_error(e)
     set_language_info(request, response, lang)
+    # Prevent expired tokens from accumulating needlessly.
+    if hasattr(request, 'session'):
+        handle_expired_openid_tokens(request.session)
     return response
 
 
@@ -263,6 +267,35 @@
         return False
 
 
+def handle_expired_openid_tokens(session):
+    now_ts = int(time.time())
+
+    # Ensure session has expirations
+    if 'token_expirations' not in session:
+        session['token_expirations'] = {}
+
+    # Remove expired tokens
+    token_expirations = session['token_expirations']
+    for token, expiration in token_expirations.items():
+        if now_ts > expiration:
+            session.pop(token, None)
+            token_expirations.pop(token, None)
+
+    # Set a 24h expiration to tokens with no expiration.
+    for key, value in session.items():
+        if (len(key) == 16 and key not in token_expirations):
+            try:
+                # Check if value is actually a valid token.
+                signed.loads(value, settings.SECRET_KEY)
+                token_expirations[key] = now_ts + settings.OPENID_TOKEN_TTL
+            except (TypeError, ValueError):
+                # Not a valid token; ignore.
+                pass
+
+    # Make sure the session is persisted.
+    session.modified = True
+
+
 def decide(request, token):
     try:
         orequest = utils.get_orequest(request, token)