[#1941] Subclass repoze.who AuthTktCookiePlugin.

Override _get_cookies to ensure the presence or absence of the HttpOnly
flag in each cookie value based on a new httponly property of the class.

ckan/config/middleware.py

@@ -18,12 +18,12 @@
 from routes.middleware import RoutesMiddleware
 from repoze.who.config import WhoConfig
 from repoze.who.middleware import PluggableAuthenticationMiddleware
-from repoze.who.plugins.auth_tkt import make_plugin as auth_tkt_make_plugin
 from fanstatic import Fanstatic
 
 from ckan.plugins import PluginImplementations
 from ckan.plugins.interfaces import IMiddleware
 from ckan.lib.i18n import get_locales_from_config
+from ckan.lib.auth_tkt import make_plugin as auth_tkt_make_plugin
 import ckan.lib.uploader as uploader
 
 from ckan.config.environment import load_environment

ckan/lib/auth_tkt.py

@@ -0,0 +1,95 @@
+import os
+
+from repoze.who.plugins import auth_tkt as repoze_auth_tkt
+
+_bool = repoze_auth_tkt._bool
+
+import logging
+log = logging.getLogger(__name__)
+
+
+class CkanAuthTktCookiePlugin(repoze_auth_tkt.AuthTktCookiePlugin):
+
+    def __init__(self, httponly, *args, **kwargs):
+        super(CkanAuthTktCookiePlugin, self).__init__(*args, **kwargs)
+        self.httponly = httponly
+
+    def _ensure_httponly_for_cookies(self, cookies):
+        '''
+        Take a list of cookie tuples items and ensure HttpOnly is set
+        correctly.
+        '''
+        # change tuples to lists
+        cookie_list = [list(c) for c in cookies]
+        # ensure httponly is set correctly for each cookie
+        for cookie in cookie_list:
+            cookie[1] = _set_substring(cookie[1], '; HttpOnly', self.httponly)
+        # change lists back to tuples and return
+        return [tuple(c) for c in cookie_list]
+
+    def _get_cookies(self, *args, **kwargs):
+        '''
+        Override method in superclass to ensure HttpOnly is set appropriately.
+        '''
+        super_cookies = super(CkanAuthTktCookiePlugin, self). \
+            _get_cookies(*args, **kwargs)
+
+        cookies = self._ensure_httponly_for_cookies(super_cookies)
+
+        return cookies
+
+
+def _set_substring(value, substring, presence=True):
+    '''
+    Ensure presence/absence of substring in value.
+
+    If presence is True, ensure value contains substring. Append substring to
+    value if absent.
+
+    If presence is False, ensure value does not contain substring. Remove
+    substring from value is present.
+    '''
+    has_substring = (substring in value)
+    if has_substring and not presence:
+        # remove substring
+        value = value.replace(substring, '')
+    elif not has_substring and presence:
+        # add substring
+        value += substring
+    return value
+
+
+def make_plugin(httponly=True,
+                secret=None,
+                secretfile=None,
+                cookie_name='auth_tkt',
+                secure=False,
+                include_ip=False,
+                timeout=None,
+                reissue_time=None,
+                userid_checker=None):
+    from repoze.who.utils import resolveDotted
+    if (secret is None and secretfile is None):
+        raise ValueError("One of 'secret' or 'secretfile' must not be None.")
+    if (secret is not None and secretfile is not None):
+        raise ValueError("Specify only one of 'secret' or 'secretfile'.")
+    if secretfile:
+        secretfile = os.path.abspath(os.path.expanduser(secretfile))
+        if not os.path.exists(secretfile):
+            raise ValueError("No such 'secretfile': %s" % secretfile)
+        secret = open(secretfile).read().strip()
+    if timeout:
+        timeout = int(timeout)
+    if reissue_time:
+        reissue_time = int(reissue_time)
+    if userid_checker is not None:
+        userid_checker = resolveDotted(userid_checker)
+    plugin = CkanAuthTktCookiePlugin(httponly,
+                                     secret,
+                                     cookie_name,
+                                     _bool(secure),
+                                     _bool(include_ip),
+                                     timeout,
+                                     reissue_time,
+                                     userid_checker)
+    return plugin

ckan/new_tests/lib/test_auth_tkt.py

@@ -0,0 +1,153 @@
+from ckan.lib.auth_tkt import CkanAuthTktCookiePlugin
+from ckan.lib.auth_tkt import _set_substring
+
+
+class TestSetSubstring(object):
+
+    '''Tests for auth_tkt._set_substring method.'''
+
+    def test_set_substring__value_has_substring_and_substring_should_be_present(self):
+        '''Substring should be retained in value'''
+        value = "I love my kitten, it is sweet."
+        substring = "kitten"
+        presence = True
+        new_value = _set_substring(value, substring, presence)
+        assert new_value == value
+
+    def test_set_substring__value_has_substring_and_substring_should_not_be_present(self):
+        '''Substring should be removed from value.'''
+        value = "I love my kitten, it is sweet."
+        substring = "kitten"
+        presence = False
+        new_value = _set_substring(value, substring, presence)
+        assert new_value == "I love my , it is sweet."
+
+    def test_set_substring__value_doesnot_have_substring_and_substring_should_be_present(self):
+        '''Substring is appended to value.'''
+        value = "I wish I had a "
+        substring = "kitten"
+        presence = True
+        new_value = _set_substring(value, substring, presence)
+        assert new_value == 'I wish I had a kitten'
+
+    def test_set_substring__value_doesnot_have_substring_and_substring_should_not_be_present(self):
+        '''Substring isn't appended to value.'''
+        value = "I don't have one."
+        substring = "kitten"
+        presence = False
+        new_value = _set_substring(value, substring, presence)
+        assert new_value == "I don't have one."
+
+
+class TestEnsureHttpOnlyForCookies(object):
+
+    '''Tests for CkanAuthTktCookiePlugin._ensure_httponly_for_cookies method'''
+
+    def _make_plugin(self, httponly):
+        '''Only httponly needs to be set.'''
+        return CkanAuthTktCookiePlugin(httponly=httponly,
+                                       secret=None,
+                                       cookie_name='auth_tkt',
+                                       secure=False,
+                                       include_ip=False,
+                                       timeout=None,
+                                       reissue_time=None,
+                                       userid_checker=None)
+
+    def test_ensure_httponly_for_cookies__should_have_httponly(self):
+        '''Cookie values should contain HttpOnly.'''
+        plugin = self._make_plugin(httponly=True)
+        cookies = [
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/'),
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/; Domain=localhost'),
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/; Domain=.localhost')
+        ]
+        ensured_cookies = plugin._ensure_httponly_for_cookies(cookies)
+        expected_cookies = [
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/; HttpOnly'),
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/; Domain=localhost; HttpOnly'),
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/; Domain=.localhost; HttpOnly')
+        ]
+        assert ensured_cookies == expected_cookies
+
+    def test_ensure_httponly_for_cookies__should_have_httponly_already_do(self):
+        '''
+        Cookie values should contain HttpOnly, they already to so nothing
+        should change.
+        '''
+        plugin = self._make_plugin(httponly=True)
+        cookies = [
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/; HttpOnly'),
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/; Domain=localhost; HttpOnly'),
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/; Domain=.localhost; HttpOnly')
+        ]
+        ensured_cookies = plugin._ensure_httponly_for_cookies(cookies)
+        assert ensured_cookies == cookies
+
+    def test_ensure_httponly_for_cookies__should_not_have_httponly_already_absent(self):
+        '''
+        Cookie values should not contain HttpOnly. They don't so nothing
+        should change.
+        '''
+        plugin = self._make_plugin(httponly=False)
+        cookies = [
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/'),
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/; Domain=localhost'),
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/; Domain=.localhost')
+        ]
+        ensured_cookies = plugin._ensure_httponly_for_cookies(cookies)
+        assert ensured_cookies == cookies
+
+    def test_ensure_httponly_for_cookies__should_not_have_httponly(self):
+        '''
+        Cookie values should not contain HttpOnly, they do so it should be
+        removed.
+        '''
+        plugin = self._make_plugin(httponly=False)
+        cookies = [
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/; HttpOnly'),
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/; Domain=localhost; HttpOnly'),
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/; Domain=.localhost; HttpOnly')
+        ]
+        ensured_cookies = plugin._ensure_httponly_for_cookies(cookies)
+        expected_cookies = [
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/'),
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/; Domain=localhost'),
+            ('Set-Cookie', 'auth_tkt="HELLO"; Path=/; Domain=.localhost')
+        ]
+        assert ensured_cookies == expected_cookies
+
+
+class TestCkanAuthTktCookiePlugin(object):
+
+    '''
+    Test the added methods used by this subclass of
+    repoze.who.plugins.auth_tkt.AuthTktCookiePlugin
+    '''
+
+    def _make_plugin(self, httponly):
+        '''Only httponly needs to be set.'''
+        return CkanAuthTktCookiePlugin(httponly=httponly,
+                                       secret=None,
+                                       cookie_name='auth_tkt',
+                                       secure=False,
+                                       include_ip=False,
+                                       timeout=None,
+                                       reissue_time=None,
+                                       userid_checker=None)
+
+    def test_httponly_present(self):
+        '''HttpOnly flag should be present in cookie values.'''
+        plugin = self._make_plugin(httponly=True)
+        cookies = plugin._get_cookies(environ={'SERVER_NAME': '0.0.0.0'},
+                                      value='ANYTHING')
+        for cookie in cookies:
+            assert 'HttpOnly' in cookie[1]
+
+    def test_httponly_absent(self):
+        '''HttpOnly flag should be absent in cookie values.'''
+        plugin = self._make_plugin(httponly=False)
+        cookies = plugin._get_cookies(environ={'SERVER_NAME': '0.0.0.0'},
+                                      value='ANYTHING')
+        for cookie in cookies:
+            assert 'HttpOnly' not in cookie[1]