From 70ffdd80a3be9cf39642aeb9fa9030ab8d718328 Mon Sep 17 00:00:00 2001
From: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date: Sun, 18 Oct 2020 12:54:50 +0200
Subject: [PATCH] auth_oidc: dont stop redirect on state lost (#47825)

Next url is usually to a local view requiring login, so on state lost we
can just continue to original next_url and it will start a new login.
---
 src/authentic2_auth_oidc/views.py | 15 +++++++++------
 tests/test_auth_oidc.py           | 23 +++++++++++++++++++++++
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/src/authentic2_auth_oidc/views.py b/src/authentic2_auth_oidc/views.py
index 49bad4e0..39da607d 100644
--- a/src/authentic2_auth_oidc/views.py
+++ b/src/authentic2_auth_oidc/views.py
@@ -41,6 +41,9 @@ def oidc_login(request, pk, next_url=None, *args, **kwargs):
     provider = get_provider(pk)
     scopes = set(provider.scopes.split()) | set(['openid'])
     state = str(uuid.uuid4())
+    next_url = next_url or request.GET.get(REDIRECT_FIELD_NAME, '')
+    if next_url and not good_next_url(request, next_url):
+        next_url = None
     nonce = request.GET.get('nonce') or str(uuid.uuid4())
     display = set()
     prompt = set()
@@ -49,7 +52,7 @@ def oidc_login(request, pk, next_url=None, *args, **kwargs):
         'scope': ' '.join(scopes),
         'response_type': 'code',
         'redirect_uri': request.build_absolute_uri(reverse('oidc-login-callback')),
-        'state': state,
+        'state': state if not next_url else state + ' ' + next_url,
         'nonce': nonce,
     }
     if provider.claims_parameter_supported:
@@ -73,8 +76,7 @@ def oidc_login(request, pk, next_url=None, *args, **kwargs):
     saved_state = request.session.setdefault('auth_oidc', {}).setdefault(state, {})
     saved_state['request'] = params
     saved_state['issuer'] = provider.issuer
-    next_url = next_url or request.GET.get(REDIRECT_FIELD_NAME, '')
-    if good_next_url(request, next_url):
+    if next_url:
         saved_state['next_url'] = next_url
     request.session.modified = True  # necessary if auth_oidc already exists
     logger.debug('auth_oidc: sent request to authorization endpoint %r', params)
@@ -102,12 +104,13 @@ class LoginCallback(View):
     def get(self, request, *args, **kwargs):
         logger = logging.getLogger(__name__)
         code = request.GET.get('code')
-        state = request.GET.get('state')
+        raw_state = request.GET.get('state')
+        state = raw_state.split(' ', 1)[0]
         oidc_state = self.oidc_state = request.session.get('auth_oidc', {}).get(state)
         if not state or not oidc_state or 'request' not in oidc_state:
-            messages.warning(request, _('Login with OpenIDConnect failed, state lost.'))
             logger.warning('auth_oidc: state lost')
-            return redirect(request, settings.LOGIN_REDIRECT_URL)
+            state_next_url = raw_state.split(' ', 1)[1] if ' ' in raw_state else None
+            return redirect(request, state_next_url or settings.LOGIN_REDIRECT_URL)
         oidc_request = oidc_state.get('request')
         assert isinstance(oidc_request, dict), 'state is not properly initialized'
         nonce = oidc_request.get('nonce')
diff --git a/tests/test_auth_oidc.py b/tests/test_auth_oidc.py
index 3c83e9eb..6dd0acf2 100644
--- a/tests/test_auth_oidc.py
+++ b/tests/test_auth_oidc.py
@@ -822,3 +822,26 @@ def test_templated_claim_mapping(app, caplog, code, oidc_provider, oidc_provider
     assert user.last_name == 'DOE'
     # typo in template string, no rendering
     assert user.first_name == '{{ given_name'
+
+
+def test_lost_state(app, caplog, code, oidc_provider, oidc_provider_jwkset, hooks):
+    response = app.get('/login/?next=/whatever/')
+    assert oidc_provider.name in response.text
+    response = response.click(oidc_provider.name)
+    qs = urlparse.parse_qs(urlparse.urlparse(response.location).query)
+    state = qs['state'][0]
+    assert ' /whatever/' in state
+    nonce = app.session['auth_oidc'][state.split()[0]]['request']['nonce']
+
+    # reset the session to forget the state
+    app.session.flush()
+
+    caplog.clear()
+    with oidc_provider_mock(oidc_provider, oidc_provider_jwkset, code, nonce=nonce):
+        response = app.get(login_callback_url(oidc_provider), params={'code': code, 'state': state})
+    # not logged
+    assert 'auth_oidc: state lost' == caplog.records[-1].message
+    # event is recorded
+    assert '_auth_user_id' not in app.session
+    # we are automatically redirected to our destination
+    assert response.location == '/whatever/'
-- 
2.28.0