launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-676372-codebrowse-openid into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #676372 "Server denied check_authentication" from bazaar.launchpad.net private branch since 11926 deployed
  https://bugs.launchpad.net/bugs/676372

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-676372-codebrowse-openid/+merge/48086

As analysed in bug #676372, loggerhead currently uses non-shared, non-persistent memory storage for OpenID associations. This doesn't work particularly well in our load-balanced environment, as the two instances are unaware of the other's associations.

This branch stops loggerhead from using an OpenID store. Ideally we would share a store between the instances, but that is non-trivial. Using no store means that the RP has to make a check_authentication request to the OP for every authentication attempt, but SSO is local so that's no issue.

There are no tests for this part of the code, and QA for this is going to be difficult due to the lack of a load-balanced environment on (qa)staging. But we can confirm that it still works on a single instance, and it can hardly be more broken than it is already.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-676372-codebrowse-openid/+merge/48086
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-676372-codebrowse-openid into lp:launchpad.

=== modified file 'lib/launchpad_loggerhead/app.py'
--- lib/launchpad_loggerhead/app.py	2010-12-17 04:48:01 +0000
+++ lib/launchpad_loggerhead/app.py	2011-01-31 23:57:56 +0000
@@ -17,7 +17,6 @@
 
 from openid.extensions.sreg import SRegRequest, SRegResponse
 from openid.consumer.consumer import CANCEL, Consumer, FAILURE, SUCCESS
-from openid.store.memstore import MemoryStore
 
 from paste.fileapp import DataApp
 from paste.request import construct_url, parse_querystring, path_info_pop
@@ -64,7 +63,6 @@
         self.branchfs = xmlrpclib.ServerProxy(
             config.codehosting.codehosting_endpoint)
         self.session_var = session_var
-        self.store = MemoryStore()
         self.log = logging.getLogger('lp-loggerhead')
 
     def get_transport(self):
@@ -76,7 +74,10 @@
 
     def _make_consumer(self, environ):
         """Build an OpenID `Consumer` object with standard arguments."""
-        return Consumer(environ[self.session_var], self.store)
+        # Multiple instances need to share a store or not use one at all (in
+        # which case they will use check_authentication). Using no store is
+        # easier, and check_authentication is cheap.
+        return Consumer(environ[self.session_var], None)
 
     def _begin_login(self, environ, start_response):
         """Start the process of authenticating with OpenID.