launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:python-openid2 into launchpad:master.

Commit message:
Upgrade to python-openid2 3.2rc2

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/387907

This adds Python 3 support.

Our fork is no longer needed, because as of e6d54f6311 we force the OpenID fetcher to Urllib2Fetcher in loggerhead as well as in lp.services.webapp.login, and the fork was only ever needed to fix CurlFetcher.

I've removed several patches for problems with python-openid that have been fixed in python-openid2.

Dependencies MP: https://code.launchpad.net/~cjwatson/lp-source-dependencies/+git/lp-source-dependencies/+merge/387906
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:python-openid2 into launchpad:master.

diff --git a/constraints.txt b/constraints.txt
index 706f7da..3b919aa 100644
--- a/constraints.txt
+++ b/constraints.txt
@@ -278,10 +278,7 @@ python-dateutil==2.8.1
 python-debian==0.1.32
 python-keystoneclient==0.7.1
 python-memcached==1.58
-# XXX: deryck 2012-08-10
-# See lp:~deryck/python-openid/python-openid-fix1034376 which
-# reapplied a patch from wgrant to get codehosting going again.
-python-openid==2.2.5-fix1034376
+python-openid2==3.2rc2
 python-swiftclient==2.0.3
 PyYAML==3.10
 rabbitfixture==0.4.2
diff --git a/lib/launchpad_loggerhead/app.py b/lib/launchpad_loggerhead/app.py
index a671d73..d6a653b 100644
--- a/lib/launchpad_loggerhead/app.py
+++ b/lib/launchpad_loggerhead/app.py
@@ -53,7 +53,6 @@ from lp.code.interfaces.codehosting import (
 from lp.codehosting.vfs import get_lp_server
 from lp.services.config import config
 from lp.services.webapp.errorlog import ErrorReportingUtility
-from lp.services.webapp.openid import set_default_openid_fetcher
 from lp.services.webapp.vhosts import allvhosts
 from lp.xmlrpc import faults
 
@@ -69,9 +68,6 @@ robots_app = DataApp(robots_txt, content_type='text/plain')
 thread_locals = threading.local()
 
 
-set_default_openid_fetcher()
-
-
 def check_fault(fault, *fault_classes):
     """Check if 'fault's faultCode matches any of 'fault_classes'.
 
@@ -124,7 +120,7 @@ class RootApp:
         openid_request = self._make_consumer(environ).begin(
             config.launchpad.openid_provider_root)
         openid_request.addExtension(
-            SRegRequest(required=['nickname']))
+            SRegRequest(required=[u'nickname']))
         back_to = construct_url(environ)
         raise HTTPMovedPermanently(openid_request.redirectURL(
             config.codehosting.secure_codebrowse_root,
diff --git a/lib/launchpad_loggerhead/wsgi.py b/lib/launchpad_loggerhead/wsgi.py
index 6002448..be5601b 100644
--- a/lib/launchpad_loggerhead/wsgi.py
+++ b/lib/launchpad_loggerhead/wsgi.py
@@ -19,7 +19,6 @@ import traceback
 
 from gunicorn.app.base import Application
 from gunicorn.glogging import Logger
-from openid import oidutil
 from paste.deploy.config import PrefixMiddleware
 from paste.httpexceptions import HTTPExceptionHandler
 from paste.request import construct_url
@@ -89,10 +88,6 @@ class LoggerheadLogger(Logger):
             ['-q', '--ms', '--log-file=DEBUG:%s' % cfg.errorlog])
         logger(log_options)
 
-        # Make the OpenID library use proper logging rather than writing to
-        # stderr.
-        oidutil.log = lambda message, level=0: log.debug(message)
-
 
 def _on_starting_hook(arbiter):
     # Normally lp.services.pidfile.make_pidfile does this, but in this case
diff --git a/lib/lp/scripts/utilities/test.py b/lib/lp/scripts/utilities/test.py
index 208c764..167d5c4 100755
--- a/lib/lp/scripts/utilities/test.py
+++ b/lib/lp/scripts/utilities/test.py
@@ -106,24 +106,6 @@ def filter_warnings():
     warnings.filterwarnings(
         'ignore', 'bzrlib.*was deprecated', DeprecationWarning,
         )
-    # The next one is caused by an infelicity in python-openid.  The
-    # following change to openid/server/server.py would make the warning
-    # filter unnecessary:
-    # 978c974,974
-    # >         try:
-    # >             namespace = request.message.getOpenIDNamespace()
-    # >         except AttributeError:
-    # >             namespace = request.namespace
-    # >         self.fields = Message(namespace)
-    # ---
-    # <         self.fields = Message(request.namespace)
-    warnings.filterwarnings(
-        'ignore',
-        (r'The \"namespace\" attribute of CheckIDRequest objects is '
-         r'deprecated.\s+'
-         r'Use \"message.getOpenIDNamespace\(\)\" instead'),
-        DeprecationWarning
-    )
     # XXX cjwatson 2019-10-18: This can be dropped once the port to Breezy
     # is complete.
     warnings.filterwarnings(
diff --git a/lib/lp/services/openid/browser/openiddiscovery.py b/lib/lp/services/openid/browser/openiddiscovery.py
index 260f57c..2fe7023 100644
--- a/lib/lp/services/openid/browser/openiddiscovery.py
+++ b/lib/lp/services/openid/browser/openiddiscovery.py
@@ -13,6 +13,7 @@ from openid.yadis.constants import (
     YADIS_CONTENT_TYPE,
     YADIS_HEADER_NAME,
     )
+import six
 
 from lp.services.openid.adapters.openid import CurrentOpenIDEndPoint
 from lp.services.propertycache import cachedproperty
@@ -55,9 +56,10 @@ class XRDSContentNegotiationMixin:
             # the value of the "Accept" header.
             self.request.response.setHeader('Vary', 'Accept')
 
-            accept_content = self.request.get('HTTP_ACCEPT', '')
+            accept_content = six.ensure_text(
+                self.request.get('HTTP_ACCEPT', ''), encoding='ISO-8859-1')
             acceptable = getAcceptable(accept_content,
-                                       ['text/html', YADIS_CONTENT_TYPE])
+                                       [u'text/html', YADIS_CONTENT_TYPE])
             # Return the XRDS document if it is preferred to text/html.
             for mtype in acceptable:
                 if mtype == 'text/html':
diff --git a/lib/lp/services/openid/extensions/macaroon.py b/lib/lp/services/openid/extensions/macaroon.py
index 30e1680..97a1731 100644
--- a/lib/lp/services/openid/extensions/macaroon.py
+++ b/lib/lp/services/openid/extensions/macaroon.py
@@ -39,7 +39,8 @@ __all__ = [
     'MacaroonResponse',
     ]
 
-from openid import oidutil
+import logging
+
 from openid.extension import Extension
 from openid.message import (
     NamespaceAliasRegistrationError,
@@ -50,10 +51,13 @@ from openid.message import (
 MACAROON_NS = 'http://ns.login.ubuntu.com/2016/openid-macaroon'
 
 
+logger = logging.getLogger(__name__)
+
+
 try:
     registerNamespaceAlias(MACAROON_NS, 'macaroon')
 except NamespaceAliasRegistrationError as e:
-    oidutil.log(
+    logger.exception(
         'registerNamespaceAlias(%r, %r) failed: %s' % (
             MACAROON_NS, 'macaroon', e))
 
diff --git a/lib/lp/services/openid/extensions/tests/test_macaroon.py b/lib/lp/services/openid/extensions/tests/test_macaroon.py
index bcbc8a8..c87f6cc 100644
--- a/lib/lp/services/openid/extensions/tests/test_macaroon.py
+++ b/lib/lp/services/openid/extensions/tests/test_macaroon.py
@@ -1,6 +1,8 @@
 # Copyright 2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+from __future__ import absolute_import, print_function, unicode_literals
+
 __metaclass__ = type
 
 from openid.consumer.consumer import SuccessResponse
diff --git a/lib/lp/services/webapp/openid.py b/lib/lp/services/openid/fetcher.py
similarity index 84%
rename from lib/lp/services/webapp/openid.py
rename to lib/lp/services/openid/fetcher.py
index 5e3f34e..3575124 100644
--- a/lib/lp/services/webapp/openid.py
+++ b/lib/lp/services/openid/fetcher.py
@@ -21,12 +21,24 @@ from openid.fetchers import (
 from six.moves.urllib.request import urlopen
 
 from lp.services.config import config
+from lp.services.encoding import wsgi_native_string
+
+
+class WSGIFriendlyUrllib2Fetcher(Urllib2Fetcher):
+
+    def fetch(self, url, body=None, headers=None):
+        if headers is not None:
+            headers = {
+                wsgi_native_string(key): wsgi_native_string(value)
+                for key, value in headers.items()}
+        return super(WSGIFriendlyUrllib2Fetcher, self).fetch(
+            url, body=body, headers=headers)
 
 
 def set_default_openid_fetcher():
     # Make sure we're using the same fetcher that we use in production, even
     # if pycurl is installed.
-    fetcher = Urllib2Fetcher()
+    fetcher = WSGIFriendlyUrllib2Fetcher()
     if config.launchpad.enable_test_openid_provider:
         # Tests have an instance name that looks like 'testrunner-appserver'
         # or similar. We're in 'development' there, so just use that config.
diff --git a/lib/lp/services/webapp/login.py b/lib/lp/services/webapp/login.py
index fdbd079..5367047 100644
--- a/lib/lp/services/webapp/login.py
+++ b/lib/lp/services/webapp/login.py
@@ -66,7 +66,6 @@ from lp.services.webapp.interfaces import (
     IPlacelessLoginSource,
     LoggedOutEvent,
     )
-from lp.services.webapp.openid import set_default_openid_fetcher
 from lp.services.webapp.publisher import LaunchpadView
 from lp.services.webapp.url import urlappend
 from lp.services.webapp.vhosts import allvhosts
@@ -160,9 +159,6 @@ def register_basiclogin(event):
             name='+basiclogin')
 
 
-set_default_openid_fetcher()
-
-
 class OpenIDLogin(LaunchpadView):
     """A view which initiates the OpenID handshake with our provider."""
     _openid_session_ns = 'OPENID'
diff --git a/lib/lp/testopenid/browser/server.py b/lib/lp/testopenid/browser/server.py
index c3d540a..f2ce757 100644
--- a/lib/lp/testopenid/browser/server.py
+++ b/lib/lp/testopenid/browser/server.py
@@ -15,7 +15,6 @@ __all__ = [
 
 from datetime import timedelta
 
-from openid import oidutil
 from openid.extensions.sreg import (
     SRegRequest,
     SRegResponse,
@@ -78,10 +77,6 @@ SESSION_PKG_KEY = 'TestOpenID'
 openid_store = MemoryStore()
 
 
-# Shut up noisy OpenID library
-oidutil.log = lambda message, level=0: None
-
-
 @implementer(ICanonicalUrlData)
 class TestOpenIDRootUrlData:
     """`ICanonicalUrlData` for the test OpenID provider."""
@@ -151,8 +146,7 @@ class OpenIDMixin:
         query = {}
         for key, value in self.request.form.items():
             if key.startswith('openid.'):
-                # All OpenID query args are supposed to be ASCII.
-                query[key.encode('US-ASCII')] = value.encode('US-ASCII')
+                query[key] = value
         return query
 
     def getSession(self):
diff --git a/lib/lp/testopenid/interfaces/server.py b/lib/lp/testopenid/interfaces/server.py
index 434fee3..669476f 100644
--- a/lib/lp/testopenid/interfaces/server.py
+++ b/lib/lp/testopenid/interfaces/server.py
@@ -9,6 +9,7 @@ __all__ = [
     'ITestOpenIDPersistentIdentity',
     ]
 
+import six
 from zope.interface import Interface
 from zope.schema import TextLine
 
@@ -36,4 +37,5 @@ def get_server_url():
     This is wrapped in a function (instead of a constant) to make sure the
     vhost.testopenid section is not required in production configs.
     """
-    return urlappend(allvhosts.configs['testopenid'].rooturl, '+openid')
+    return six.ensure_text(
+        urlappend(allvhosts.configs['testopenid'].rooturl, '+openid'))
diff --git a/lib/lp/testopenid/stories/basics.txt b/lib/lp/testopenid/stories/basics.txt
index 56c6f4e..6672e40 100644
--- a/lib/lp/testopenid/stories/basics.txt
+++ b/lib/lp/testopenid/stories/basics.txt
@@ -37,7 +37,7 @@ POST request.
     >>> print anon_browser.headers
     Status: 200 Ok
     ...
-    Content-Type: text/plain
+    Content-Type: text/plain;charset=utf-8
     ...
     >>> print anon_browser.contents
     assoc_handle:{HMAC-SHA1}{...}{...}
diff --git a/lib/lp/testopenid/testing/helpers.py b/lib/lp/testopenid/testing/helpers.py
index 27f9855..99cd52d 100644
--- a/lib/lp/testopenid/testing/helpers.py
+++ b/lib/lp/testopenid/testing/helpers.py
@@ -21,6 +21,7 @@ from openid.consumer.discover import (
 from six.moves.urllib.error import HTTPError
 from zope.testbrowser.wsgi import Browser
 
+from lp.services.encoding import wsgi_native_string
 from lp.services.webapp import LaunchpadView
 from lp.testopenid.interfaces.server import get_server_url
 
@@ -44,8 +45,8 @@ class ZopeFetcher(fetchers.HTTPFetcher):
         browser = Browser()
         if headers is not None:
             for key, value in headers.items():
-                browser.addHeader(key, value)
-        browser.addHeader('X-Zope-Handle-Errors', 'True')
+                browser.addHeader(key, wsgi_native_string(value))
+        browser.addHeader('X-Zope-Handle-Errors', wsgi_native_string('True'))
         try:
             browser.open(url, data=body)
         except HTTPError as e:
diff --git a/lib/lp_sitecustomize.py b/lib/lp_sitecustomize.py
index d23cd74..26085c0 100644
--- a/lib/lp_sitecustomize.py
+++ b/lib/lp_sitecustomize.py
@@ -21,6 +21,7 @@ from lp.services.log import loglevels
 from lp.services.log.logger import LaunchpadLogger
 from lp.services.log.mappingfilter import MappingFilter
 from lp.services.mime import customizeMimetypes
+from lp.services.openid.fetcher import set_default_openid_fetcher
 
 
 def add_custom_loglevels():
@@ -178,6 +179,7 @@ def main(instance_name=None):
     customizeMimetypes()
     silence_warnings()
     customize_logger()
+    set_default_openid_fetcher()
     checker.BasicTypes.update({defaultdict: checker.NoProxy})
     checker.BasicTypes.update({Deferred: checker.NoProxy})
     checker.BasicTypes.update({DeferredList: checker.NoProxy})
diff --git a/setup.py b/setup.py
index 111ac58..e152c90 100644
--- a/setup.py
+++ b/setup.py
@@ -213,7 +213,7 @@ setup(
         'python-keystoneclient',
         'python-memcached',
         'python-mimeparse',
-        'python-openid',
+        'python-openid2',
         'python-subunit',
         'python-swiftclient',
         'pytz',