Merge lp:~salgado/launchpad/separate-cookie-for-testopenid into lp:launchpad

[Guilherme Salgado]

[Guilherme Salgado (salgado)]

This branch changes our custom CookieClientIDManager to allow us to use a separate cookie for requests in the TestOpenIDLayer. This is needed so that the cookie you get when logging into testopenid.lp.dev is not picked up in lp.dev

[Guilherme Salgado (salgado)]

This might not be needed, so not yet ready for review.

[Gary Poster (gary)]

Good.

As we discussed on IRC, we both agree that it will be nice to revert this once it is no longer necessary.

Because of that, and for general readability, I'd like to have the cut-n-paste blocks from Zope code more clearly marked. You have a nice comment at the end of one of those blocks ("# From here on it's our custom code...") but I'd like to see a start marker and end marker for these blocks, and a comment that this change is slated to be reverted.

Other than that, I'm happy with it.

Thanks,

Gary

review: Approve

[Guilherme Salgado (salgado)]

I've made it clear where the duplication is and filed bug 520582 (linking to this MP), so that I don't forget to revert the changes later on.

Unmerged revisions

10218. By Guilherme Salgado on 2010-02-11

merge from loomified branch

10217. By Guilherme Salgado on 2010-02-11

merge from mainline

=== modified file 'lib/canonical/config/schema-lazr.conf'
--- lib/canonical/config/schema-lazr.conf	2010-02-11 16:33:20 +0000
+++ lib/canonical/config/schema-lazr.conf	2010-02-11 16:33:22 +0000
@@ -1114,6 +1114,11 @@
 # datatype: string
 cookie: launchpad
 
+# The id of the cookie used to store the session token for the testopenid
+# service.
+# datatype: string
+testopenid_cookie: testopenid
+
 
 [librarianlogparser]
 logs_root = /srv/launchpadlibrarian.net-logs

=== modified file 'lib/canonical/launchpad/webapp/authentication.py'
--- lib/canonical/launchpad/webapp/authentication.py	2009-10-20 22:13:21 +0000
+++ lib/canonical/launchpad/webapp/authentication.py	2010-02-11 16:33:22 +0000
@@ -22,6 +22,7 @@
 from zope.interface import implements
 from zope.component import getUtility
 from zope.event import notify
+from zope.session.interfaces import IClientIdManager
 
 from zope.security.proxy import removeSecurityProxy
 
@@ -29,7 +30,6 @@
 from zope.app.security.interfaces import ILoginPassword
 from zope.app.security.principalregistry import UnauthenticatedPrincipal
 
-from canonical.config import config
 from canonical.launchpad.interfaces.account import IAccountSet
 from canonical.launchpad.interfaces.launchpad import IPasswordEncryptor
 from canonical.launchpad.interfaces.oauth import OAUTH_CHALLENGE
@@ -119,7 +119,7 @@
             # Hack to make us not even think of using a session if there
             # isn't already a cookie in the request, or one waiting to be
             # set in the response.
-            cookie_name = config.launchpad_session.cookie
+            cookie_name = getUtility(IClientIdManager).getNamespace(request)
             if (request.cookies.get(cookie_name) is not None or
                 request.response.getCookie(cookie_name) is not None):
                 return self._authenticateUsingCookieAuth(request)

=== modified file 'lib/canonical/launchpad/webapp/dbpolicy.py'
--- lib/canonical/launchpad/webapp/dbpolicy.py	2010-01-20 22:09:26 +0000
+++ lib/canonical/launchpad/webapp/dbpolicy.py	2010-02-11 16:33:22 +0000
@@ -184,7 +184,7 @@
 
     def _hasSession(self):
         "Is there is already a session cookie hanging around?"
-        cookie_name = getUtility(IClientIdManager).namespace
+        cookie_name = getUtility(IClientIdManager).getNamespace(self.request)
         return (
             cookie_name in self.request.cookies or
             self.request.response.getCookie(cookie_name) is not None)
@@ -325,7 +325,7 @@
         # standard Launchpad database policy for load balancing to
         # the slave databases. The javascript web service libraries
         # send the session cookie for authenticated users.
-        cookie_name = getUtility(IClientIdManager).namespace
+        cookie_name = getUtility(IClientIdManager).getNamespace(request)
         if cookie_name in request.cookies:
             return LaunchpadDatabasePolicy(request)
         # Otherwise, use the master only web service database policy.

=== modified file 'lib/canonical/launchpad/webapp/login.py'
--- lib/canonical/launchpad/webapp/login.py	2010-01-14 13:25:34 +0000
+++ lib/canonical/launchpad/webapp/login.py	2010-02-11 16:33:22 +0000
@@ -471,7 +471,7 @@
                         delta=timedelta(minutes=10)):
     if client_id_manager is None:
         client_id_manager = getUtility(IClientIdManager)
-    session_cookiename = client_id_manager.namespace
+    session_cookiename = client_id_manager.getNamespace(request)
     value = request.response.getCookie(session_cookiename)['value']
     expiration = (datetime.utcnow() + delta).strftime(
         '%a, %d %b %Y %H:%M:%S GMT')
@@ -492,7 +492,8 @@
     if not IUnauthenticatedPrincipal.providedBy(request.principal):
         return
     client_id_manager = getUtility(IClientIdManager)
-    if request.response.getCookie(client_id_manager.namespace) is None:
+    cookie_name = client_id_manager.getNamespace(request)
+    if request.response.getCookie(cookie_name) is None:
         client_id_manager.setRequestId(
             request, client_id_manager.getClientId(request))
         expireSessionCookie(request, client_id_manager, duration)

=== modified file 'lib/canonical/launchpad/webapp/pgsession.py'
--- lib/canonical/launchpad/webapp/pgsession.py	2009-11-30 08:57:54 +0000
+++ lib/canonical/launchpad/webapp/pgsession.py	2010-02-11 16:33:22 +0000
@@ -115,9 +115,9 @@
             if IUnauthenticatedPrincipal.providedBy(request.principal):
                 # it would be nice if this could be a monitored, logged
                 # message instead of an instant-OOPS.
-                assert (client_id_manager.namespace in request.cookies or
-                        request.response.getCookie(
-                            client_id_manager.namespace) is not None), (
+                namespace = client_id_manager.getNamespace(request)
+                assert (namespace in request.cookies or
+                        request.response.getCookie(namespace) is not None), (
                     'Session data should generally only be stored for '
                     'authenticated users, and for users who have just logged '
                     'out.  If an unauthenticated user has just logged out, '

=== modified file 'lib/canonical/launchpad/webapp/session.py'
--- lib/canonical/launchpad/webapp/session.py	2009-06-25 05:30:52 +0000
+++ lib/canonical/launchpad/webapp/session.py	2010-02-11 16:33:22 +0000
@@ -5,13 +5,21 @@
 
 __metaclass__ = type
 
+import hmac
+import logging
+import time
+from email.utils import formatdate
+from hashlib import sha1
+
 from cookielib import domain_match
 from zope.component import getUtility
-from zope.session.http import CookieClientIdManager
+from zope.publisher.interfaces.http import IHTTPApplicationRequest
+from zope.session.http import CookieClientIdManager, digestEncode
 
 from storm.zope.interfaces import IZStorm
 
 from canonical.config import config
+from canonical.launchpad.layers import TestOpenIDLayer
 from canonical.launchpad.webapp.url import urlparse
 
 
@@ -20,6 +28,7 @@
 HOURS = 60 * MINUTES
 DAYS = 24 * HOURS
 YEARS = 365 * DAYS
+logger = logging.getLogger()
 
 
 def get_cookie_domain(request_domain):
@@ -45,13 +54,33 @@
 class LaunchpadCookieClientIdManager(CookieClientIdManager):
 
     def __init__(self):
-        CookieClientIdManager.__init__(self)
-        self.namespace = config.launchpad_session.cookie
+        # Don't upcall to the parent here as it will only set self.namespace
+        # and self.secret, which we don't want as we need to use
+        # getNamespace(request) and we have a @property for secret.
+
         # Set the cookie life time to something big.
         # It should be larger than our session expiry time.
         self.cookieLifetime = 1 * YEARS
         self._secret = None
 
+    @property
+    def namespace(self):
+        raise AttributeError(
+            "This class doesn't provide a namespace attribute because it "
+            "needs a request to figure out the correct namespace. Use "
+            "getNamespace(request) instead.")
+
+    def getNamespace(self, request):
+        """Return the correct namespace according to the given request.
+
+        This is needed so that we can use a separate cookie for the testopenid
+        vhost.
+        """
+        if TestOpenIDLayer.providedBy(request):
+            return config.launchpad_session.testopenid_cookie
+        else:
+            return config.launchpad_session.cookie
+
     def getClientId(self, request):
         sid = self.getRequestId(request)
         if sid is None:
@@ -69,7 +98,8 @@
                 request.annotations[ANNOTATION_KEY] = sid
         return sid
 
-    def _get_secret(self):
+    @property
+    def secret(self):
         # Because our CookieClientIdManager is not persistent, we need to
         # pull the secret from some other data store - failing to do this
         # would mean a new secret is generated every time the server is
@@ -82,16 +112,44 @@
             self._secret = result.get_one()[0]
         return self._secret
 
-    def _set_secret(self, value):
-        # Silently ignored so CookieClientIdManager's __init__ method
-        # doesn't die
-        pass
+    # Re-implement the parent's method so that we can use
+    # self.getNamespace(request) instead of self.namespace.
+    def getRequestId(self, request):
+        """Return the browser id encoded in request as a string
 
-    secret = property(_get_secret, _set_secret)
+        Return None if an id is not set.
+        """
+        cookie_name = self.getNamespace(request)
+        response_cookie = request.response.getCookie(cookie_name)
+        if response_cookie:
+            sid = response_cookie['value']
+        else:
+            request = IHTTPApplicationRequest(request)
+            sid = request.getCookies().get(cookie_name, None)
+        if self.thirdparty:
+            return sid
+        else:
+            # If there is an id set on the response, use that but
+            # don't trust it.  We need to check the response in case
+            # there has already been a new session created during the
+            # course of this request.
+            if sid is None or len(sid) != 54:
+                return None
+            s, mac = sid[:27], sid[27:]
+            
+            # call s.encode() to workaround a bug where the hmac
+            # module only accepts str() types in Python 2.6
+            if (digestEncode(hmac.new(
+                    s.encode(), self.secret, digestmod=sha1
+                ).digest()) != mac):
+                return None
+            else:
+                return sid
 
     def setRequestId(self, request, id):
-        """As per CookieClientIdManager.setRequestID, except
-        we force the domain key on the cookie to be set to allow our
+        """Set cookie with id on request.
+
+        Also force the domain key on the cookie to be set to allow our
         session to be shared between virtual hosts where possible, and
         we set the secure key to stop the session key being sent to
         insecure URLs like the Librarian.
@@ -99,12 +157,46 @@
         We also log the referrer url on creation of a new
         requestid so we can track where first time users arrive from.
         """
-        # XXX: SteveAlexander, 2007-04-01.
-        #      This is on the codepath where anon users get a session cookie
-        #      set unnecessarily.
-        CookieClientIdManager.setRequestId(self, request, id)
-
-        cookie = request.response.getCookie(self.namespace)
+        # TODO: Currently, the path is the ApplicationURL. This is reasonable,
+        #     and will be adequate for most purposes.
+        #     A better path to use would be that of the folder that contains
+        #     the site manager this service is registered within. However,
+        #     that would be expensive to look up on each request, and would
+        #     have to be altered to take virtual hosting into account.
+        #     Seeing as this utility instance has a unique namespace for its
+        #     cookie, using ApplicationURL shouldn't be a problem.
+
+        if self.thirdparty:
+            logger.warning(
+                'ClientIdManager is using thirdparty cookies, ignoring '
+                'setRequestId call')
+            return
+
+        response = request.response
+        options = {}
+        if self.cookieLifetime is not None:
+            if self.cookieLifetime:
+                expires = formatdate(time.time() + self.cookieLifetime,
+                                     localtime=False, usegmt=True)
+            else:
+                expires = 'Tue, 19 Jan 2038 00:00:00 GMT'
+
+            options['expires'] = expires
+
+        response.setCookie(
+            self.getNamespace(request), id,
+            path=request.getApplicationURL(path_only=True),
+            **options)
+
+        response.setHeader(
+            'Cache-Control', 'no-cache="Set-Cookie,Set-Cookie2"')
+        response.setHeader('Pragma', 'no-cache')
+        response.setHeader('Expires', 'Mon, 26 Jul 1997 05:00:00 GMT')
+
+        # From here on it's our custom code -- everything else in this method
+        # is copied from zope.session.http, replacing just self.namespace with
+        # self.getNamespace(request).
+        cookie = request.response.getCookie(self.getNamespace(request))
         protocol, request_domain = urlparse(request.getURL())[:2]
 
         # Set secure flag on cookie.

=== added file 'lib/canonical/launchpad/webapp/tests/test_cookie_client_id_manager.txt'
--- lib/canonical/launchpad/webapp/tests/test_cookie_client_id_manager.txt	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/webapp/tests/test_cookie_client_id_manager.txt	2010-02-11 16:33:22 +0000
@@ -0,0 +1,158 @@
+= Cookie Client ID Manager =
+
+We use a custom ICookieClientIdManager which instead of a 'namespace'
+attribute provides a getNamespace(request) method. That's because we sometimes
+need to change the namespace depending on the request's layer (e.g. when the
+request is on the TestOpenID layer we want a separate cookie from the
+Launchpad one).
+
+Thanks to this requirement, our custom class has to re-implement a couple
+methods from CookieClientIdManager that expect the 'namespace' attribute to
+be provided.
+
+    >>> from canonical.launchpad.webapp.session import (
+    ...   LaunchpadCookieClientIdManager)
+    >>> bim = LaunchpadCookieClientIdManager()
+    >>> bim.namespace
+    Traceback (most recent call last):
+    ...
+    AttributeError: This class doesn't provide a namespace attribute...
+
+    >>> from StringIO import StringIO
+    >>> from zope.publisher.http import HTTPRequest
+    >>> environ = {'SERVER_URL': 'http://launchpad.dev'}
+    >>> request = HTTPRequest(StringIO(''), environ, None)
+    >>> bim.getNamespace(request)
+    'launchpad_tests'
+
+When the request is in the TestOpenIDLayer, a separate cookie is used.
+
+    >>> from canonical.launchpad.layers import setFirstLayer, TestOpenIDLayer
+    >>> testopenid_request = HTTPRequest(StringIO(''), environ, None)
+    >>> setFirstLayer(testopenid_request, TestOpenIDLayer)
+    >>> bim.getNamespace(testopenid_request)
+    'testopenid'
+
+
+== getRequestId(request) ==
+
+Because no cookie has been set, we get no id:
+
+    >>> bim.getRequestId(request) is None
+    True
+
+We can set an id:
+
+    >>> id1 = bim.generateUniqueId()
+    >>> bim.setRequestId(request, id1)
+
+And get it back:
+
+    >>> bim.getRequestId(request) == id1
+    True
+
+When we set the request id, we also set a response cookie.  We
+can simulate getting this cookie back in a subsequent request:
+
+    >>> request2 = HTTPRequest(StringIO(''), environ, None)
+    >>> request2._cookies = dict(
+    ...   [(name, cookie['value'])
+    ...    for (name, cookie) in request.response._cookies.items()
+    ...   ])
+
+And we get the same id back from the new request:
+
+    >>> bim.getRequestId(request) == bim.getRequestId(request2)
+    True
+
+Test a corner case where Python 2.6 hmac module does not allow
+unicode as input:
+
+    >>> id_uni = unicode(bim.generateUniqueId())
+    >>> bim.setRequestId(request, id_uni)
+    >>> bim.getRequestId(request) == id_uni
+    True
+
+If another server is managing the ClientId cookies (Apache, Nginx)
+we're returning their value without checking:
+
+    >>> bim.thirdparty = True
+    >>> request3 = HTTPRequest(StringIO(''), environ, None)
+    >>> request3._cookies = {
+    ...     bim.getNamespace(request3): 'AQAAf0Y4gjgAAAQ3AwMEAg=='}
+    >>> bim.getRequestId(request3)
+    'AQAAf0Y4gjgAAAQ3AwMEAg=='
+
+
+== setRequestId(request, id) ==
+
+Note that the id is checked for validity. Setting an
+invalid value is silently ignored:
+
+    >>> request = HTTPRequest(StringIO(''), environ, None)
+    >>> bim = LaunchpadCookieClientIdManager()
+    >>> bim.getRequestId(request)
+    >>> bim.setRequestId(request, 'invalid id')
+    >>> bim.getRequestId(request)
+
+For now, the cookie path is the application URL:
+
+    >>> cookie = request.response.getCookie(bim.getNamespace(request))
+    >>> cookie['path'] == request.getApplicationURL(path_only=True)
+    True
+
+By default, session cookies have a lifetime of 1 year:
+
+    >>> bim.cookieLifetime # 1 year in seconds.
+    31536000
+    >>> cookie.has_key('expires')
+    True
+
+Expiry time of 0 means never (well - close enough)
+
+    >>> bim.cookieLifetime = 0
+    >>> request = HTTPRequest(StringIO(''), environ, None)
+    >>> sid = bim.getRequestId(request)
+    >>> bim.setRequestId(request, sid)
+    >>> cookie = request.response.getCookie(bim.getNamespace(request))
+    >>> cookie['expires']
+    'Tue, 19 Jan 2038 00:00:00 GMT'
+
+If another server in front of Zope (Apache, Nginx) is managing the
+cookies we won't set any ClientId cookies:
+
+    >>> request = HTTPRequest(StringIO(''), environ, None)
+    >>> bim.thirdparty = True
+    >>> bim.setRequestId(request, '1234')
+    WARNING:root:ClientIdManager is using thirdparty cookies...
+    >>> cookie = request.response.getCookie(bim.getNamespace(request))
+    >>> print cookie
+    None
+
+For https requests, the secure cookie option is included.
+
+    >>> bim.thirdparty = False
+    >>> bim.cookieLifetime = None
+    >>> request = HTTPRequest(
+    ...     StringIO(''), {'SERVER_URL': 'https://example.com'}, None)
+    >>> bim.setRequestId(request, '1234')
+    >>> print request.response.getCookie(bim.getNamespace(request))
+    {'path': '/', 'secure': True, 'value': '1234'}
+
+But for http requests, it is not.
+
+    >>> request = HTTPRequest(
+    ...     StringIO(''), {'SERVER_URL': 'http://example.com'}, None)
+    >>> bim.setRequestId(request, '1234')
+    >>> print request.response.getCookie(bim.getNamespace(request))
+    {'path': '/', 'secure': False, 'value': '1234'}
+
+When the cookie is set, cache headers are added to the
+response to try to prevent the cookie header from being cached:
+
+    >>> request.response.getHeader('Cache-Control')
+    'no-cache="Set-Cookie,Set-Cookie2"'
+    >>> request.response.getHeader('Pragma')
+    'no-cache'
+    >>> request.response.getHeader('Expires')
+    'Mon, 26 Jul 1997 05:00:00 GMT'

=== modified file 'lib/canonical/launchpad/webapp/tests/test_dbpolicy.py'
--- lib/canonical/launchpad/webapp/tests/test_dbpolicy.py	2010-01-13 20:06:09 +0000
+++ lib/canonical/launchpad/webapp/tests/test_dbpolicy.py	2010-02-11 16:33:22 +0000
@@ -191,7 +191,7 @@
         newInteraction(request)
         try:
             # First, generate a valid session cookie.
-            cookie_name = getUtility(IClientIdManager).namespace
+            cookie_name = getUtility(IClientIdManager).getNamespace(request)
             ISession(request)['whatever']['whatever'] = 'whatever'
             # Then stuff it into the request where we expect to
             # find it. The database policy is only interested if

=== modified file 'lib/canonical/launchpad/webapp/tests/test_session.py'
--- lib/canonical/launchpad/webapp/tests/test_session.py	2009-06-25 05:30:52 +0000
+++ lib/canonical/launchpad/webapp/tests/test_session.py	2010-02-11 16:33:22 +0000
@@ -3,6 +3,9 @@
 
 import unittest
 
+from canonical.testing import DatabaseFunctionalLayer
+from canonical.launchpad.testing.systemdocs import (
+    LayeredDocFileSuite, setUp, tearDown)
 from canonical.launchpad.webapp.session import get_cookie_domain
 
 
@@ -44,4 +47,9 @@
 
 
 def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)
+    suite = unittest.TestSuite()
+    suite.addTest(LayeredDocFileSuite(
+        'test_cookie_client_id_manager.txt',
+        layer=DatabaseFunctionalLayer, setUp=setUp, tearDown=tearDown))
+    suite.addTests(unittest.TestLoader().loadTestsFromName(__name__))
+    return suite