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

Proposed by Maximiliano Bertacchini on 2019-09-25
Status: Merged
Approved by: Maximiliano Bertacchini on 2019-09-26
Approved revision: 1699
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: lp:~maxiberta/canonical-identity-provider/openid-session-limit
Merge into: lp:canonical-identity-provider/release
Diff against target: 188 lines (+93/-8)
3 files modified
django_project/settings_base.py (+2/-1)
src/identityprovider/tests/test_views_server.py (+60/-5)
src/identityprovider/views/server.py (+31/-2)
To merge this branch: bzr merge lp:~maxiberta/canonical-identity-provider/openid-session-limit
Reviewer Review Type Date Requested Status
Daniel Manrique 2019-09-25 Approve on 2019-09-25
Review via email: mp+373232@code.launchpad.net

Commit message

Limit the number of OpenID tokens stored in user session.

Description of the change

As a follow-up to openid token expiration[0], this limits the maximum number of tokens that can be stored in a user session. Older tokens (by expiration date) are deleted first. This will prevent the DB from growing infinitely.

The default number (10) should be enough for users with multiple sessions/devices/browsers scenarios. Can be configured with juju[1].

[0] https://code.launchpad.net/~maxiberta/canonical-identity-provider/openid-session-ttl/+merge/354127
[1] https://code.launchpad.net/~maxiberta/canonical-is-charms/canonical-identity-provider_add-openid_session_limit/+merge/373234

To post a comment you must log in.
Daniel Manrique (roadmr) wrote :

+1 from me code-wise, I made some observations re: a comment and a test.

About multiple session/device/browser - I was thinking this is probably not an issue, because each different browser/device will get an entirely new Django session; we discussed an example with 5 devices and calculated 2 openid "sessions" per device, but in reality each device would have its own django "session" containing up to 10 openid "sessions" which does seem a bit excessive. Still, the value of 10 is a good starting point and we could tweak using the Juju setting.

review: Approve
1698. By Maximiliano Bertacchini on 2019-09-26

Improve comment around OPENID_TOKEN_LIMIT.

1699. By Maximiliano Bertacchini on 2019-09-26

Update tests on openid token limit.

Maximiliano Bertacchini (maxiberta) wrote :

QA instructions, for posterity:

# Run python-openid's example consumer
env/bin/python branches/python-openid/examples/consumer.py

# Go to http://localhost:8001
# Use identifier e.g. https://staging.launchpad.net/~maxiberta-h

# Run a django management shell on staging SSO (stg-sso@wendigo)
juju ssh sso-app/0
cd /srv/login.staging.ubuntu.com/staging/code/current
sudo su ubunet
PYTHONPATH=../../etc env/bin/python django_project/manage.py shell

# Query sessions with multiple openid tokens
from django.contrib.sessions.models import Session
from pprint import pprint
pprint([(s.session_key, s.expire_date, sorted(s.get_decoded().get('token_expirations').items(), key=lambda x: x[1]), sorted(s.get_decoded().keys())) for s in Session.objects.all() if len(s.get_decoded().get('token_expirations', [])) > 1])

# Retry the openid dance and re-print sessions: see how openid tokens in the session accumulate up to 10, then start rotating by expiration date.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'django_project/settings_base.py'
2--- django_project/settings_base.py 2019-05-08 11:01:47 +0000
3+++ django_project/settings_base.py 2019-09-26 13:23:06 +0000
4@@ -438,11 +438,12 @@
5 OPENID_LAUNCHPAD_TEAMS_REQUIRED = []
6 OPENID_PHYSICAL_MULTIFACTOR_REQUIRED = False
7 OPENID_PREAUTHORIZATION_ACL = []
8-OPENID_TOKEN_TTL = 24 * 60 * 60 # 1 day, seconds
9 OPENID_SREG_EXTRA_FIELDS = ['language']
10 OPENID_SREG_REQUIRED_FIELDS = []
11 OPENID_SSO_SERVER_URL = SSO_ROOT_URL
12 OPENID_STRICT_USERNAMES = False
13+OPENID_TOKEN_LIMIT = 10
14+OPENID_TOKEN_TTL = 24 * 60 * 60 # 1 day, seconds
15 OPENID_TRUST_ROOT = SSO_ROOT_URL
16 OPENID_UPDATE_DETAILS_FROM_SREG = True
17 OPENID_USE_AS_ADMIN_LOGIN = False
18
19=== modified file 'src/identityprovider/tests/test_views_server.py'
20--- src/identityprovider/tests/test_views_server.py 2019-04-01 18:04:15 +0000
21+++ src/identityprovider/tests/test_views_server.py 2019-09-26 13:23:06 +0000
22@@ -1,5 +1,5 @@
23 # -*- coding: utf-8 -*-
24-# Copyright 2010-2018 Canonical Ltd. This software is licensed under
25+# Copyright 2010-2019 Canonical Ltd. This software is licensed under
26 # the GNU Affero General Public License version 3 (see the file
27 # LICENSE).
28
29@@ -7,6 +7,7 @@
30 import time
31 import urlparse
32
33+from collections import OrderedDict
34 from urllib import quote, quote_plus, urlencode
35
36 from django.conf import settings
37@@ -640,13 +641,13 @@
38 self.assertFalse(valid)
39
40
41-class ExpiredTokensHandlerTestCase(SSOBaseTestCase):
42+class TokenCleanupTestCase(SSOBaseTestCase):
43
44 def _generate_openid_token(self, session, token=None, expiration=None):
45 if token is None:
46 token = generate_random_string(16)
47 session[token] = signed.dumps('orequest', settings.SECRET_KEY)
48- if expiration:
49+ if expiration is not None:
50 if 'token_expirations' not in session:
51 session['token_expirations'] = {}
52 session['token_expirations'][token] = expiration
53@@ -731,6 +732,35 @@
54 self.assertIn('token_expirations', session)
55 self.assertFalse(session['token_expirations'])
56
57+ def test_excess_token_cleaning_below_limit(self):
58+ session = self.client.session
59+ for i in range(settings.OPENID_TOKEN_LIMIT - 1):
60+ expiration = int(time.time()) - i # `i` seconds ago
61+ self._generate_openid_token(session, expiration=expiration)
62+
63+ server.handle_excess_openid_tokens(session)
64+
65+ self.assertEqual(
66+ len(session['token_expirations']), settings.OPENID_TOKEN_LIMIT - 1)
67+
68+ def test_excess_token_cleaning_over_limit(self):
69+ session = self.client.session
70+ token_expirations = OrderedDict()
71+ for i in range(settings.OPENID_TOKEN_LIMIT + 1):
72+ expiration = int(time.time()) - i # `i` seconds ago
73+ token = self._generate_openid_token(session, expiration=expiration)
74+ token_expirations[token] = expiration
75+
76+ server.handle_excess_openid_tokens(session)
77+
78+ self.assertEqual(
79+ len(session['token_expirations']), settings.OPENID_TOKEN_LIMIT)
80+ # Tokens are ordered by expiration (per OrderedDict), older last
81+ expected_token_expirations = dict(list(
82+ token_expirations.items())[:settings.OPENID_TOKEN_LIMIT])
83+ self.assertEqual(
84+ session['token_expirations'], expected_token_expirations)
85+
86
87 class DecideBaseTestCase(AuthenticatedTestCase):
88
89@@ -2027,8 +2057,7 @@
90 self.assertTrue(r)
91
92 session = self.client.session
93- session[token] = signed.dumps(orequest,
94- settings.SECRET_KEY)
95+ session[token] = signed.dumps(orequest, settings.SECRET_KEY)
96 session.save()
97
98 r = self.client.get("/%s/+untrusted" % token)
99@@ -2052,6 +2081,32 @@
100 self.assertEqual(len(session['token_expirations']), 1)
101 self.assertIn(session['token_expirations'].keys()[0], session)
102
103+ def test_excess_token_cleanup(self):
104+ params = {'openid.mode': 'checkid_setup',
105+ 'openid.trust_root': 'http://localhost/',
106+ 'openid.return_to': 'http://localhost/',
107+ 'openid.identity': IDENTIFIER_SELECT}
108+ request = self.factory.make_request(**params)
109+ provider_url = get_provider_url(request)
110+
111+ session = self.client.session
112+
113+ self.assertNotIn('token_expirations', session)
114+
115+ session['token_expirations'] = {}
116+ for i in range(1, settings.OPENID_TOKEN_LIMIT + 1):
117+ token = generate_random_string(16)
118+ expiration = int(time.time()) + i * 10 # within i * 10 secs
119+ session[token] = signed.dumps('orequest', settings.SECRET_KEY)
120+ session['token_expirations'][token] = expiration
121+ session.save()
122+
123+ response = self.client.get(provider_url, params)
124+
125+ session = response.client.session
126+ self.assertEqual(
127+ len(session['token_expirations']), settings.OPENID_TOKEN_LIMIT)
128+
129
130 class MarkupTestCase(AuthenticatedTestCase):
131
132
133=== modified file 'src/identityprovider/views/server.py'
134--- src/identityprovider/views/server.py 2018-09-07 14:19:01 +0000
135+++ src/identityprovider/views/server.py 2019-09-26 13:23:06 +0000
136@@ -1,4 +1,4 @@
137-# Copyright 2010-2018 Canonical Ltd. This software is licensed under
138+# Copyright 2010-2019 Canonical Ltd. This software is licensed under
139 # the GNU Affero General Public License version 3 (see the file
140 # LICENSE).
141
142@@ -141,9 +141,10 @@
143 except ProtocolError as e:
144 response = _handle_openid_error(e)
145 set_language_info(request, response, lang)
146- # Prevent expired tokens from accumulating needlessly.
147+ # Prevent expired and too many tokens from accumulating needlessly.
148 if hasattr(request, 'session'):
149 handle_expired_openid_tokens(request.session)
150+ handle_excess_openid_tokens(request.session)
151 return response
152
153
154@@ -296,6 +297,34 @@
155 session.modified = True
156
157
158+def handle_excess_openid_tokens(session):
159+ # Assume `handle_expired_openid_tokens()` was run first.
160+ # This guarantees token_expirations exists for all tokens in the session.
161+
162+ # Do nothing if we have less than `OPENID_TOKEN_LIMIT` openid tokens
163+ if len(session['token_expirations']) <= settings.OPENID_TOKEN_LIMIT:
164+ return
165+
166+ # Get a list of token IDs sorted by token_expiration, older ones last
167+ keys_by_time = [
168+ item[0]
169+ for item in sorted(
170+ session['token_expirations'].items(),
171+ key=lambda item: item[1],
172+ reverse=True,
173+ )
174+ ]
175+
176+ # Each openid data item is about 4kB. Leave at most `OPENID_TOKEN_LIMIT`
177+ # of them so maximum session size from openid is about 40kB.
178+ for token in keys_by_time[settings.OPENID_TOKEN_LIMIT:]:
179+ session.pop(token, None)
180+ session['token_expirations'].pop(token, None)
181+
182+ # Make sure the session is persisted.
183+ session.modified = True
184+
185+
186 def decide(request, token):
187 try:
188 orequest = utils.get_orequest(request, token)