From eded66558a10f3e04a2f632a81751b7b8ea57914 Mon Sep 17 00:00:00 2001 From: Benjamin Dauvergne Date: Fri, 27 Nov 2020 08:07:49 +0100 Subject: [PATCH] idp_oidc: make access_token validity depends on expiration or session existence (#48889) --- .../migrations/0014_auto_20201126_1812.py | 18 ++++++ src/authentic2_idp_oidc/models.py | 36 ++++++----- src/authentic2_idp_oidc/views.py | 27 +++++--- tests/test_idp_oidc.py | 64 ++++++++++++++++++- 4 files changed, 118 insertions(+), 27 deletions(-) create mode 100644 src/authentic2_idp_oidc/migrations/0014_auto_20201126_1812.py diff --git a/src/authentic2_idp_oidc/migrations/0014_auto_20201126_1812.py b/src/authentic2_idp_oidc/migrations/0014_auto_20201126_1812.py new file mode 100644 index 00000000..c037e1d5 --- /dev/null +++ b/src/authentic2_idp_oidc/migrations/0014_auto_20201126_1812.py @@ -0,0 +1,18 @@ +# Generated by Django 2.2.17 on 2020-11-26 17:12 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('authentic2_idp_oidc', '0013_auto_20200630_1007'), + ] + + operations = [ + migrations.AlterField( + model_name='oidcaccesstoken', + name='expired', + field=models.DateTimeField(null=True, verbose_name='expire'), + ), + ] diff --git a/src/authentic2_idp_oidc/models.py b/src/authentic2_idp_oidc/models.py index 0da97aaa..ae1d23ce 100644 --- a/src/authentic2_idp_oidc/models.py +++ b/src/authentic2_idp_oidc/models.py @@ -24,6 +24,7 @@ from django.core.exceptions import ValidationError, ImproperlyConfigured from django.utils.translation import ugettext_lazy as _ from django.conf import settings from django.utils import six +from django.utils.functional import cached_property from django.utils.timezone import now from django.utils.six.moves.urllib import parse as urlparse from django.contrib.contenttypes.fields import GenericForeignKey, GenericRelation @@ -56,6 +57,15 @@ def strip_words(data): return u'\n'.join([url for url in data.split()]) +def get_session(session_key): + engine = import_module(settings.SESSION_ENGINE) + session = engine.SessionStore(session_key=session_key) + session.load() + if session._session_key == session_key: + return session + return None + + class OIDCClient(Service): POLICY_UUID = 1 POLICY_PAIRWISE = 2 @@ -332,15 +342,9 @@ class OIDCCode(models.Model): objects = managers.OIDCExpiredManager() - @property + @cached_property def session(self): - if not hasattr(self, '_session'): - engine = import_module(settings.SESSION_ENGINE) - session = engine.SessionStore(session_key=self.session_key) - session.load() - if session._session_key == self.session_key: - self._session = session - return getattr(self, '_session', None) + return self.session_key and get_session(self.session_key) def scope_set(self): return utils.scope_set(self.scopes) @@ -382,24 +386,22 @@ class OIDCAccessToken(models.Model): verbose_name=_('created'), auto_now_add=True) expired = models.DateTimeField( - verbose_name=_('expire')) + verbose_name=_('expire'), + null=True) objects = managers.OIDCExpiredManager() def scope_set(self): return utils.scope_set(self.scopes) - @property + @cached_property def session(self): - if not hasattr(self, '_session'): - engine = import_module(settings.SESSION_ENGINE) - session = engine.SessionStore(session_key=self.session_key) - if session._session_key == self.session_key: - self._session = session - return getattr(self, '_session', None) + return self.session_key and get_session(self.session_key) def is_valid(self): - return self.expired >= now() and self.session is not None + return ((self.expired is None or self.expired >= now()) + and (self.session_key == '' + or get_session(self.session_key) is not None)) def __repr__(self): return '' % ( diff --git a/src/authentic2_idp_oidc/views.py b/src/authentic2_idp_oidc/views.py index 4253a59b..42260d3e 100644 --- a/src/authentic2_idp_oidc/views.py +++ b/src/authentic2_idp_oidc/views.py @@ -101,10 +101,6 @@ def idtoken_duration(client): return client.idtoken_duration or datetime.timedelta(seconds=app_settings.IDTOKEN_DURATION) -def access_token_duration(client): - return client.access_token_duration or datetime.timedelta(seconds=app_settings.IDTOKEN_DURATION) - - def allowed_scopes(client): return client.scope_set() or app_settings.SCOPES or ['openid', 'email', 'profile'] @@ -328,14 +324,19 @@ def authorize(request, *args, **kwargs): else: # FIXME: we should probably factorize this part with the token endpoint similar code need_access_token = 'token' in response_type.split() - expires_in = access_token_duration(client) if need_access_token: + if client.access_token_duration is None: + expires_in = datetime.timedelta(request.session.get_expiry_age()) + expired = None + else: + expires_in = client.access_token_duration + expired = start + client.access_token_duration access_token = models.OIDCAccessToken.objects.create( client=client, user=request.user, scopes=u' '.join(scopes), session_key=request.session.session_key, - expired=start + expires_in) + expired=expired) acr = '0' if nonce is not None and last_auth.get('nonce') == nonce: acr = '1' @@ -510,7 +511,10 @@ def idtoken_from_user_credential(request): exponential_backoff.success(*backoff_keys) start = now() # make access_token - expires_in = access_token_duration(client) + if client.access_token_duration is None: + expires_in = datetime.timedelta(seconds=app_settings.ACCESS_TOKEN_DURATION) + else: + expires_in = client.access_token_duration access_token = models.OIDCAccessToken.objects.create( client=client, user=user, @@ -559,13 +563,18 @@ def tokens_from_authz_code(request): redirect_uri = request.POST.get('redirect_uri') if oidc_code.redirect_uri != redirect_uri: return invalid_request_response('invalid redirect_uri') - expires_in = access_token_duration(client) + if client.access_token_duration is None: + expires_in = datetime.timedelta(oidc_code.session.get_expiry_age()) + expired = None + else: + expires_in = client.access_token_duration + expired = oidc_code.created + expires_in access_token = models.OIDCAccessToken.objects.create( client=client, user=oidc_code.user, scopes=oidc_code.scopes, session_key=oidc_code.session_key, - expired=oidc_code.created + expires_in) + expired=expired) start = now() acr = '0' if (oidc_code.nonce is not None diff --git a/tests/test_idp_oidc.py b/tests/test_idp_oidc.py index 3bb06f47..b3fd027c 100644 --- a/tests/test_idp_oidc.py +++ b/tests/test_idp_oidc.py @@ -15,8 +15,9 @@ # along with this program. If not, see . import base64 -import json import datetime +from importlib import import_module +import json import pytest @@ -1689,3 +1690,64 @@ def test_oidc_authorized_oauth_services_view(app, oidc_client, simple_user): 'button', {'class': 'authorized-oauth-services--revoke-button'})) == 1 assert OIDCAuthorization.objects.filter( client_ct=ContentType.objects.get_for_model(OU)).count() == 0 + + +@pytest.fixture +def session(settings, db): + engine = import_module(settings.SESSION_ENGINE) + session = engine.SessionStore() + session.create() + return session + + +def test_access_token_is_valid_session(simple_oidc_client, simple_user, session): + token = OIDCAccessToken.objects.create( + client=simple_oidc_client, + user=simple_user, + scopes='openid', + session_key=session.session_key) + + assert token.is_valid() + session.flush() + assert not token.is_valid() + + +def test_access_token_is_valid_expired(simple_oidc_client, simple_user, freezer): + start = now() + expired = start + datetime.timedelta(seconds=30) + + token = OIDCAccessToken.objects.create( + client=simple_oidc_client, + user=simple_user, + scopes='openid', + expired=expired) + + assert token.is_valid() + freezer.move_to(expired) + assert token.is_valid() + freezer.move_to(expired + datetime.timedelta(seconds=1)) + assert not token.is_valid() + + +def test_access_token_is_valid_session_and_expired(simple_oidc_client, + simple_user, + session, freezer): + start = now() + expired = start + datetime.timedelta(seconds=30) + + token = OIDCAccessToken.objects.create( + client=simple_oidc_client, + user=simple_user, + scopes='openid', + session_key=session.session_key, + expired=expired) + + assert token.is_valid() + freezer.move_to(expired) + assert token.is_valid() + freezer.move_to(expired + datetime.timedelta(seconds=1)) + assert not token.is_valid() + freezer.move_to(start) + assert token.is_valid() + session.flush() + assert not token.is_valid() -- 2.29.2