launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/services/webapp/login.py'
> --- lib/lp/services/webapp/login.py	2016-05-19 02:02:39 +0000
> +++ lib/lp/services/webapp/login.py	2017-01-14 15:45:28 +0000
> @@ -160,11 +159,7 @@
>              name='+basiclogin')
>  
>  
> -# The Python OpenID package uses pycurl by default, but pycurl chokes on
> -# self-signed certificates (like the ones we use when developing), so we
> -# change the default to urllib2 here.  That's also a good thing because it
> -# ensures we test the same thing that we run on production.

I have no words.

> -setDefaultFetcher(Urllib2Fetcher())
> +set_default_openid_fetcher()
>  
>  
>  class OpenIDLogin(LaunchpadView):
> 
> === added file 'lib/lp/services/webapp/openid.py'
> --- lib/lp/services/webapp/openid.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/services/webapp/openid.py	2017-01-14 15:45:28 +0000
> @@ -0,0 +1,35 @@
> +# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""OpenID consumer configuration."""
> +
> +from __future__ import absolute_import, print_function, unicode_literals
> +
> +__metaclass__ = type
> +
> +__all__ = [
> +    'set_default_openid_fetcher',
> +    ]
> +
> +from functools import partial
> +import os.path
> +import urllib2
> +
> +from openid.fetchers import (
> +    setDefaultFetcher,
> +    Urllib2Fetcher,
> +    )
> +
> +from lp.services.config import config
> +
> +
> +# The Python OpenID package uses pycurl by default, but pycurl chokes on
> +# self-signed certificates (like the ones we use when developing), so we
> +# change the default to urllib2 here.  That's also a good thing because it
> +# ensures we test the same thing that we run on production.

Is this comment still relevant now that urllib2 clearly does check certificates?

> +def set_default_openid_fetcher():
> +    fetcher = Urllib2Fetcher()
> +    if config.launchpad.enable_test_openid_provider:
> +        cafile = os.path.join(config.root, "configs/development/launchpad.crt")
> +        fetcher.urlopen = partial(urllib2.urlopen, cafile=cafile)
> +    setDefaultFetcher(fetcher)


-- 
https://code.launchpad.net/~cjwatson/launchpad/testopenid-certificate/+merge/314768
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.