From 60a5236f75f6c07ace87367abf3cca8ce1a9db69 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 | 11 ++-- src/authentic2_idp_oidc/views.py | 27 +++++--- tests/test_idp_oidc.py | 63 +++++++++++++++++++ 4 files changed, 106 insertions(+), 13 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 fb5e2961..054abcd0 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 @@ -407,7 +408,8 @@ class OIDCAccessToken(SessionMixin, models.Model): verbose_name=_('created'), auto_now_add=True) expired = models.DateTimeField( - verbose_name=_('expire')) + verbose_name=_('expire'), + null=True) objects = managers.OIDCExpiredManager() @@ -415,13 +417,14 @@ class OIDCAccessToken(SessionMixin, models.Model): return utils.scope_set(self.scopes) def is_valid(self): - if self.expired < now(): + if self.expired is not None and self.expired < now(): return False if not self.session_key: return True - if not self.session: + session = get_session(self.session_key) + if session is None: return False - if self.session.get('_auth_user_id') != str(self.user_id): + if session.get('_auth_user_id') != str(self.user_id): return False return True diff --git a/src/authentic2_idp_oidc/views.py b/src/authentic2_idp_oidc/views.py index d8657b96..d7eeb1e4 100644 --- a/src/authentic2_idp_oidc/views.py +++ b/src/authentic2_idp_oidc/views.py @@ -172,10 +172,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'] @@ -405,14 +401,19 @@ def authorize_for_client(request, client, redirect_uri): response = redirect(request, redirect_uri, params=params, resolve=False) else: 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 = iat + 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=iat + expires_in) + expired=expired) acr = '0' if nonce is not None and last_auth.get('nonce') == nonce: acr = '1' @@ -601,7 +602,10 @@ def idtoken_from_user_credential(request): exponential_backoff.success(*backoff_keys) iat = now() # iat = issued at # 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, @@ -648,13 +652,18 @@ def tokens_from_authz_code(request): redirect_uri = request.POST.get('redirect_uri') if oidc_code.redirect_uri != redirect_uri: raise InvalidRequest(_('Parameter "redirect_uri" does not match the code.'), client=client) - 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 a4377763..31eb5c1e 100644 --- a/tests/test_idp_oidc.py +++ b/tests/test_idp_oidc.py @@ -17,6 +17,7 @@ import base64 import datetime import functools +from importlib import import_module import json import pytest @@ -1857,3 +1858,65 @@ def test_user_info(app, client, access_token, freezer): response['WWW-Authenticate'] == 'Bearer error="invalid_token", error_description="Token is expired or user is disconnected"' ) + + +@pytest.fixture +def session(settings, db, simple_user): + engine = import_module(settings.SESSION_ENGINE) + session = engine.SessionStore() + session['_auth_user_id'] = str(simple_user.id) + 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