From dab3cfb93bfeaa14c281e8246e688cc00b631e6f Mon Sep 17 00:00:00 2001
From: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date: Thu, 15 Nov 2018 09:41:29 +0100
Subject: [PATCH 2/2] idp_oidc: never use an invalid redirect_uri (fixes
 #28029)

Check of "redirect_uri" move earlier during authorization request
processing. For any redirect_uri check failure errors are only shown to
the end user and redirect_uri is never used to redirect to the
requesting RP.
---
 src/authentic2_idp_oidc/views.py | 22 +++++++++------
 tests/test_idp_oidc.py           | 46 +++++++++++++++++++-------------
 2 files changed, 41 insertions(+), 27 deletions(-)

diff --git a/src/authentic2_idp_oidc/views.py b/src/authentic2_idp_oidc/views.py
index ed7ce26d..137282cb 100644
--- a/src/authentic2_idp_oidc/views.py
+++ b/src/authentic2_idp_oidc/views.py
@@ -87,12 +87,23 @@ def authorize(request, *args, **kwargs):
         client_id = request.GET['client_id']
         redirect_uri = request.GET['redirect_uri']
     except KeyError as k:
-        return HttpResponseBadRequest('invalid request: missing parameter %s' % k.args[0],
-                                      content_type='text/plain')
+        messages.warning(request, _('Authorization request is invalid'))
+        logger.warning(u'idp_oidc: authorization request error, missing %s', k.args[0])
+        return redirect(request, 'auth_homepage')
     try:
         client = models.OIDCClient.objects.get(client_id=client_id)
     except models.OIDCClient.DoesNotExist:
-        return HttpResponseBadRequest('invalid request: unknown client_id', content_type='text/plain')
+        messages.warning(request, _('Authorization request is invalid'))
+        logger.warning(u'idp_oidc: authorization request error, unknown client_id redirect_uri=%r client_id=%r',
+                       redirect_uri, client_id)
+        return redirect(request, 'auth_homepage')
+
+    if redirect_uri not in client.redirect_uris.split():
+        messages.warning(request, _('Authorization request is invalid'))
+        logger.warning(u'idp_oidc: authorization request error, unknown redirect_uri redirect_uri=%r client_id=%r',
+                       redirect_uri, client_id)
+        return redirect(request, 'auth_homepage')
+
     fragment = client.authorization_flow == client.FLOW_IMPLICIT
 
     state = request.GET.get('state')
@@ -122,11 +133,6 @@ def authorize(request, *args, **kwargs):
                                        state=state,
                                        fragment=fragment)
 
-    if redirect_uri not in client.redirect_uris.split():
-        return authorization_error(request, redirect_uri, 'invalid_request',
-                                   error_description='unauthorized redirect_uri',
-                                   state=state,
-                                   fragment=fragment)
     if client.authorization_flow == client.FLOW_AUTHORIZATION_CODE:
         if response_type != 'code':
             return authorization_error(request, redirect_uri, 'unsupported_response_type',
diff --git a/tests/test_idp_oidc.py b/tests/test_idp_oidc.py
index 8ec90a9b..a27f4986 100644
--- a/tests/test_idp_oidc.py
+++ b/tests/test_idp_oidc.py
@@ -344,19 +344,23 @@ def test_invalid_request(caplog, oidc_settings, oidc_client, simple_user, app):
     else:
         raise NotImplementedError
 
-    # client_id
+    # missing client_id
     authorize_url = make_url('oidc-authorize', params={})
 
-    response = app.get(authorize_url, status=400)
-    assert 'missing parameter \'client_id\'' in response.content
+    response = app.get(authorize_url, status=302)
+    assert urlparse.urlparse(response['Location']).path  == '/'
+    response = response.maybe_follow()
+    assert 'Authorization request is invalid' in response
 
-    # redirect_uri
+    # missing redirect_uri
     authorize_url = make_url('oidc-authorize', params={
         'client_id': oidc_client.client_id,
     })
 
-    response = app.get(authorize_url, status=400)
-    assert 'missing parameter \'redirect_uri\'' in response.content
+    response = app.get(authorize_url, status=302)
+    assert urlparse.urlparse(response['Location']).path == '/'
+    response = response.maybe_follow()
+    assert 'Authorization request is invalid' in response
 
     # invalid client_id
     authorize_url = make_url('oidc-authorize', params={
@@ -364,8 +368,23 @@ def test_invalid_request(caplog, oidc_settings, oidc_client, simple_user, app):
         'redirect_uri': redirect_uri,
     })
 
-    response = app.get(authorize_url, status=400)
-    assert 'unknown client_id' in response.content
+    response = app.get(authorize_url, status=302)
+    assert urlparse.urlparse(response['Location']).path == '/'
+    response = response.maybe_follow()
+    assert 'Authorization request is invalid' in response
+
+    # invalid redirect_uri
+    authorize_url = make_url('oidc-authorize', params={
+        'client_id': oidc_client.client_id,
+        'redirect_uri': 'xxx',
+        'response_type': 'code',
+        'scope': 'openid',
+    })
+
+    response = app.get(authorize_url, status=302)
+    assert urlparse.urlparse(response['Location']).path == '/'
+    response = response.maybe_follow()
+    assert 'Authorization request is invalid' in response
 
     # missing response_type
     authorize_url = make_url('oidc-authorize', params={
@@ -411,17 +430,6 @@ def test_invalid_request(caplog, oidc_settings, oidc_client, simple_user, app):
     response = app.get(authorize_url)
     assert_oidc_error(response, 'invalid_request', 'max_age is not', fragment=fragment)
 
-    # invalid redirect_uri
-    authorize_url = make_url('oidc-authorize', params={
-        'client_id': oidc_client.client_id,
-        'redirect_uri': 'xxx',
-        'response_type': 'code',
-        'scope': 'openid',
-    })
-
-    response = app.get(authorize_url)
-    assert_oidc_error(response, 'invalid_request', 'unauthorized redirect_uri', fragment=fragment)
-
     # unsupported response_type
     authorize_url = make_url('oidc-authorize', params={
         'client_id': oidc_client.client_id,
-- 
2.18.0