Merge pull request #173 from gertjanol/bugfix-keyerror-on-cookie

Use value from setting to determine browser state

docs/sections/sessionmanagement.rst

@@ -18,6 +18,10 @@ Somewhere in your Django ``settings.py``::
 
     OIDC_SESSION_MANAGEMENT_ENABLE = True
 
+
+If you're in a multi-server setup, you might also want to add ``OIDC_UNAUTHENTICATED_SESSION_MANAGEMENT_KEY`` to your settings and set it to some random but fixed string. While authenticated clients have a session that can be used to calculate the browser state, there is no such thing for unauthenticated clients. Hence this value. By default a value is generated randomly on startup, so this will be different on each server. To get a consistent value across all servers you should set this yourself.
+
+
 Example RP iframe
 =================
 

docs/sections/settings.rst

@@ -110,6 +110,13 @@ OPTIONAL. ``bool``. Enables OpenID Connect Session Management 1.0 in your provid
 
 Default is ``False``.
 
+OIDC_UNAUTHENTICATED_SESSION_MANAGEMENT_KEY
+===========================================
+
+OPTIONAL. Supply a fixed string to use as browser-state key for unauthenticated clients. Read :ref:`sessionmanagement` section.
+
+Default is a string generated at startup.
+
 OIDC_SKIP_CONSENT_ALWAYS
 ========================
 

oidc_provider/lib/endpoints/authorize.py

@@ -30,8 +30,7 @@ from oidc_provider.models import (
     UserConsent,
 )
 from oidc_provider import settings
-from oidc_provider.lib.utils.common import cleanup_url_from_query_string
-
+from oidc_provider.lib.utils.common import cleanup_url_from_query_string, get_browser_state_or_default
 
 logger = logging.getLogger(__name__)
 
@@ -197,7 +196,7 @@ class AuthorizeEndpoint(object):
                 session_state = '{client_id} {origin} {browser_state} {salt}'.format(
                     client_id=self.client.client_id,
                     origin=client_origin,
-                    browser_state=self.request.COOKIES['op_browser_state'],
+                    browser_state=get_browser_state_or_default(self.request),
                     salt=salt)
                 session_state = sha256(session_state.encode('utf-8')).hexdigest()
                 session_state += '.' + salt

oidc_provider/lib/utils/common.py

@@ -1,3 +1,5 @@
+from hashlib import sha224
+
 from django.core.urlresolvers import reverse
 from django.http import HttpResponse
 
@@ -50,6 +52,7 @@ def get_site_url(site_url=None, request=None):
                         'or set `SITE_URL` in settings, '
                         'or pass `request` object.')
 
+
 def get_issuer(site_url=None, request=None):
     """
     Construct the issuer full url. Basically is the site url with some path
@@ -125,3 +128,11 @@ def default_idtoken_processing_hook(id_token, user):
     :rtype dict
     """
     return id_token
+
+
+def get_browser_state_or_default(request):
+    """
+    Determine value to use as session state.
+    """
+    key = request.session.session_key or settings.get('OIDC_UNAUTHENTICATED_SESSION_MANAGEMENT_KEY')
+    return sha224(key.encode('utf-8')).hexdigest()

oidc_provider/middleware.py

@@ -1,17 +1,21 @@
-from hashlib import sha224
+try:
+    # https://docs.djangoproject.com/en/1.10/topics/http/middleware/#upgrading-pre-django-1-10-style-middleware
+    from django.utils.deprecation import MiddlewareMixin
+except ImportError:
+    MiddlewareMixin = object
 
-from django.conf import settings as django_settings
-from django.utils.deprecation import MiddlewareMixin
+from oidc_provider import settings
+from oidc_provider.lib.utils.common import get_browser_state_or_default
 
 
 class SessionManagementMiddleware(MiddlewareMixin):
     """
     Maintain a `op_browser_state` cookie along with the `sessionid` cookie that
     represents the End-User's login state at the OP. If the user is not logged
-    in then use `SECRET_KEY` value.
+    in then use the value of settings.OIDC_UNAUTHENTICATED_SESSION_MANAGEMENT_KEY.
     """
 
     def process_response(self, request, response):
-        session_state = sha224((request.session.session_key or django_settings.SECRET_KEY).encode('utf-8')).hexdigest()
-        response.set_cookie('op_browser_state', session_state)
+        if settings.get('OIDC_SESSION_MANAGEMENT_ENABLE'):
+            response.set_cookie('op_browser_state', get_browser_state_or_default(request))
         return response

oidc_provider/settings.py

@@ -1,4 +1,6 @@
 import importlib
+import random
+import string
 
 from django.conf import settings
 
@@ -6,6 +8,9 @@ from django.conf import settings
 class DefaultSettings(object):
     required_attrs = ()
 
+    def __init__(self):
+        self._unauthenticated_session_management_key = None
+
     @property
     def OIDC_LOGIN_URL(self):
         """
@@ -74,6 +79,18 @@ class DefaultSettings(object):
         """
         return False
 
+    @property
+    def OIDC_UNAUTHENTICATED_SESSION_MANAGEMENT_KEY(self):
+        """
+        OPTIONAL. Supply a fixed string to use as browser-state key for unauthenticated clients.
+        """
+
+        # Memoize generated value
+        if not self._unauthenticated_session_management_key:
+            self._unauthenticated_session_management_key = ''.join(
+                random.choice(string.ascii_uppercase + string.digits) for _ in range(100))
+        return self._unauthenticated_session_management_key
+
     @property
     def OIDC_SKIP_CONSENT_ALWAYS(self):
         """

oidc_provider/tests/test_authorize_endpoint.py

@@ -7,7 +7,7 @@ try:
 except ImportError:
     from urlparse import parse_qs, urlsplit
 import uuid
-from mock import patch
+from mock import patch, mock
 
 from django.contrib.auth.models import AnonymousUser
 from django.core.management import call_command
@@ -537,3 +537,14 @@ class TestCreateResponseURI(TestCase):
             authorization_endpoint.create_response_uri()
 
         log_exception.assert_called_once_with('[Authorize] Error when trying to create response uri: %s', exception)
+
+    @override_settings(OIDC_SESSION_MANAGEMENT_ENABLE=True)
+    def test_create_response_uri_generates_session_state_if_session_management_enabled(self):
+        # RequestFactory doesn't support sessions, so we mock it
+        self.request.session = mock.Mock(session_key=None)
+
+        authorization_endpoint = AuthorizeEndpoint(self.request)
+        authorization_endpoint.validate_params()
+
+        uri = authorization_endpoint.create_response_uri()
+        self.assertIn('session_state=', uri)

oidc_provider/tests/test_middleware.py

@@ -0,0 +1,39 @@
+from django.conf.urls import url
+from django.test import TestCase, override_settings
+from django.views.generic import View
+from mock import mock
+
+
+class StubbedViews:
+    class SampleView(View):
+        pass
+
+    urlpatterns = [url('^test/', SampleView.as_view())]
+
+MW_CLASSES = ('django.contrib.sessions.middleware.SessionMiddleware',
+              'oidc_provider.middleware.SessionManagementMiddleware')
+
+
+@override_settings(ROOT_URLCONF=StubbedViews,
+                   MIDDLEWARE=MW_CLASSES,
+                   MIDDLEWARE_CLASSES=MW_CLASSES,
+                   OIDC_SESSION_MANAGEMENT_ENABLE=True)
+class MiddlewareTestCase(TestCase):
+
+    def setUp(self):
+        patcher = mock.patch('oidc_provider.middleware.get_browser_state_or_default')
+        self.mock_get_state = patcher.start()
+
+    def test_session_management_middleware_sets_cookie_on_response(self):
+        response = self.client.get('/test/')
+
+        self.assertIn('op_browser_state', response.cookies)
+        self.assertEqual(response.cookies['op_browser_state'].value,
+                         str(self.mock_get_state.return_value))
+        self.mock_get_state.assert_called_once_with(response.wsgi_request)
+
+    @override_settings(OIDC_SESSION_MANAGEMENT_ENABLE=False)
+    def test_session_management_middleware_does_not_set_cookie_if_session_management_disabled(self):
+        response = self.client.get('/test/')
+
+        self.assertNotIn('op_browser_state', response.cookies)

oidc_provider/tests/test_settings.py

@@ -8,8 +8,18 @@ CUSTOM_TEMPLATES = {
 }
 
 
-class TokenTest(TestCase):
+class SettingsTest(TestCase):
 
     @override_settings(OIDC_TEMPLATES=CUSTOM_TEMPLATES)
     def test_override_templates(self):
         self.assertEqual(settings.get('OIDC_TEMPLATES'), CUSTOM_TEMPLATES)
+
+    def test_unauthenticated_session_management_key_has_default(self):
+        key = settings.get('OIDC_UNAUTHENTICATED_SESSION_MANAGEMENT_KEY')
+        self.assertRegexpMatches(key, r'[a-zA-Z0-9]+')
+        self.assertGreater(len(key), 50)
+
+    def test_unauthenticated_session_management_key_has_constant_value(self):
+        key1 = settings.get('OIDC_UNAUTHENTICATED_SESSION_MANAGEMENT_KEY')
+        key2 = settings.get('OIDC_UNAUTHENTICATED_SESSION_MANAGEMENT_KEY')
+        self.assertEqual(key1, key2)

oidc_provider/tests/test_utils.py

@@ -1,13 +1,15 @@
 import time
 from datetime import datetime
+from hashlib import sha224
 
-from django.test import TestCase
+from django.http import HttpRequest
+from django.test import TestCase, override_settings
 from django.utils import timezone
+from mock import mock
 
-from oidc_provider.lib.utils.common import get_issuer
+from oidc_provider.lib.utils.common import get_issuer, get_browser_state_or_default
 from oidc_provider.lib.utils.token import create_id_token
 from oidc_provider.tests.app.utils import create_fake_user
-from django.test import override_settings
 
 
 class Request(object):
@@ -78,3 +80,19 @@ class TokenTest(TestCase):
             'iss': 'http://localhost:8000/openid',
             'sub': str(self.user.id),
         })
+
+
+class BrowserStateTest(TestCase):
+
+    @override_settings(OIDC_UNAUTHENTICATED_SESSION_MANAGEMENT_KEY='my_static_key')
+    def test_get_browser_state_uses_value_from_settings_to_calculate_browser_state(self):
+        request = HttpRequest()
+        request.session = mock.Mock(session_key=None)
+        state = get_browser_state_or_default(request)
+        self.assertEqual(state, sha224('my_static_key'.encode('utf-8')).hexdigest())
+
+    def test_get_browser_state_uses_session_key_to_calculate_browser_state_if_available(self):
+        request = HttpRequest()
+        request.session = mock.Mock(session_key='my_session_key')
+        state = get_browser_state_or_default(request)
+        self.assertEqual(state, sha224('my_session_key'.encode('utf-8')).hexdigest())