Merge lp:~mandel/ubuntu-sso-client/fix-alecus-mess into lp:ubuntu-sso-client

[Manuel de la Peña]

- Changed the way in which we deal with proxies to work around bugs found in the QNetworkAccessManager (LP: #957313).

- Changed the way in which we deal with proxies to work around bugs found in the QNetworkAccessManager (LP: #957313).

[Alejandro J. Cura (alecu)]

Ran tests, extensively tested IRL, and code looks as good as if I had written half of it.
Very well done!

review: Approve

[Roberto Alsina (ralsina)]

+1 code review, trust alecu's IRL

review: Approve

=== modified file 'ubuntu_sso/utils/webclient/__init__.py'
--- ubuntu_sso/utils/webclient/__init__.py	2012-03-19 15:08:56 +0000
+++ ubuntu_sso/utils/webclient/__init__.py	2012-03-19 23:59:18 +0000
@@ -34,10 +34,12 @@
 
     try:
         from PyQt4.QtCore import QCoreApplication
+        from PyQt4.QtGui import QApplication
 
         # we could be running a process with or without ui, and those are diff
         # apps.
-        result = QCoreApplication.instance() is not None
+        result = (QCoreApplication.instance() is not None
+                   or QApplication.instance() is not None)
     except ImportError:
         pass
 

=== modified file 'ubuntu_sso/utils/webclient/common.py'
--- ubuntu_sso/utils/webclient/common.py	2012-03-19 15:08:56 +0000
+++ ubuntu_sso/utils/webclient/common.py	2012-03-19 23:59:18 +0000
@@ -178,15 +178,21 @@
         from ubuntu_sso.keyring import Keyring
         keyring = Keyring()
         try:
-            creds = yield keyring.get_credentials(domain)
+            creds = yield keyring.get_credentials(str(domain))
             logger.debug('Got credentials from keyring.')
         except Exception, e:
             logger.error('Error when retrieving the creds.')
             raise WebClientError('Error when retrieving the creds.', e)
         if creds is not None:
-            self.proxy_username = creds['username']
-            self.proxy_password = creds['password']
-            defer.returnValue(True)
+            # if we are loading the same creds it means that we got the wrong
+            # ones
+            if (self.proxy_username == creds['username'] and
+                    self.proxy_password == creds['password']):
+                defer.returnValue(False)
+            else:
+                self.proxy_username = creds['username']
+                self.proxy_password = creds['password']
+                defer.returnValue(True)
         logger.debug('Proxy creds not in keyring.')
         defer.returnValue(False)
 

=== modified file 'ubuntu_sso/utils/webclient/qtnetwork.py'
--- ubuntu_sso/utils/webclient/qtnetwork.py	2012-03-19 16:43:22 +0000
+++ ubuntu_sso/utils/webclient/qtnetwork.py	2012-03-19 23:59:18 +0000
@@ -48,107 +48,75 @@
 logger = setup_logging("ubuntu_sso.utils.webclient.qtnetwork")
 
 
-class WebClientProxyFactory(QNetworkProxyFactory):
-    """Proxy factory used by the web client.
-
-    The proxy factory ensures that the correct proxy is used according to
-    the request performed by the web client. For example, if the system has
-    different proxies set for the http and the https the proxy factory will
-    return the correct proxy according to the scheme of the request.
-    """
-
-    def __init__(self):
-        """Create a new instance."""
-        super(WebClientProxyFactory, self).__init__()
-        self._proxies = dict(default=QNetworkProxy(QNetworkProxy.DefaultProxy))
-        self._setup_proxies()
-
-    def _get_proxy_for_settings(self, settings):
-        """Return a proxy that uses the given settings."""
-        if "username" in settings and "password" in settings:
-            logger.debug('Setting auth %s:%s proxy',
-                    settings.get("host", ""), settings.get("port", 0))
-            proxy = QNetworkProxy(QNetworkProxy.HttpProxy,
-                                  hostName=settings.get("host", ""),
-                                  port=settings.get("port", 0),
-                                  user=settings.get("username", ""),
-                                  password=settings.get("password", ""))
-        else:
-            logger.debug('Using nonauth proxy %s:%s', settings.get("host", ""),
-                    settings.get("port", 0))
-            proxy = QNetworkProxy(QNetworkProxy.HttpProxy,
-                                  hostName=settings.get("host", ""),
-                                  port=settings.get("port", 0))
-        return proxy
-
-    def _setup_proxies(self):
-        """Set the different proxies used in the system."""
-        proxy_settings = gsettings.get_proxy_settings()
-        for scheme, settings in proxy_settings.iteritems():
-            self._proxies[scheme] = self._get_proxy_for_settings(settings)
-
-    # ignore the complain since we have to use the Qt naming convention
-    # pylint: disable=C0103
-    def queryProxy(self, query):
-        """Return the proxy servers to be used in order of preference."""
-        if 'force' in self._proxies:
-            return [self._proxies['force']]
-        if self._proxies:
-            scheme = str(query.protocolTag())
-            if scheme in self._proxies:
-                return [self._proxies[scheme]]
-        # this function most return always at least one item, since we do not
-        # have a proxy set for the query let's return the system default proxy
-        return [self._proxies['default']]
-    # pylint: enable=C0103
-
-    def update_proxy(self, scheme, settings):
-        """Allows to update the proxy to be used for a scheme."""
-        self._proxies[scheme] = self._get_proxy_for_settings(settings)
-
-    def force_use_proxy(self, settings):
-        """Force to use the proxies in the given settings."""
-        self._proxies['force'] = self._get_proxy_for_settings(settings)
-
-    def find_proxy_schemes(self, proxy):
-        """Return the scheme in wich the proxy is used."""
-        schemes = []
-        for scheme, scheme_proxy in self._proxies.iteritems():
-            if scheme_proxy == proxy:
-                schemes.append(scheme)
-        return schemes
-
-    def find_proxy_for_scheme(self, scheme):
-        """Return the proxy used for the scheme or default otherwise."""
-        if scheme in self._proxies:
-            return self._proxies[scheme]
-        return self._proxies['default']
+def build_proxy(settings_groups):
+    """Create a QNetworkProxy from these settings."""
+    proxy_groups = [
+        ("socks", QNetworkProxy.Socks5Proxy),
+        ("https", QNetworkProxy.HttpProxy),
+        ("http", QNetworkProxy.HttpProxy),
+    ]
+    for group, proxy_type in proxy_groups:
+        if group not in settings_groups:
+            continue
+        settings = settings_groups[group]
+        if "host" in settings and "port" in settings:
+            return QNetworkProxy(proxy_type,
+                                 hostName=settings.get("host", ""),
+                                 port=settings.get("port", 0),
+                                 user=settings.get("username", ""),
+                                 password=settings.get("password", ""))
+    logger.error("No proxy correctly configured.")
+    return QNetworkProxy(QNetworkProxy.DefaultProxy)
 
 
 class WebClient(BaseWebClient):
     """A webclient with a qtnetwork backend."""
 
+    proxy_instance = None
+
     def __init__(self, *args, **kwargs):
         """Initialize this instance."""
         super(WebClient, self).__init__(*args, **kwargs)
         self.nam = QNetworkAccessManager(QCoreApplication.instance())
         self.nam.finished.connect(self._handle_finished)
         self.nam.authenticationRequired.connect(self._handle_authentication)
+        self.nam.proxyAuthenticationRequired.connect(self.handle_proxy_auth)
         # Disabled until we make this a per-instance option
         #self.nam.sslErrors.connect(self._handle_ssl_errors)
         self.replies = {}
-        self.proxy_factory = None
+        self.proxy_retry = False
         self.setup_proxy()
 
     def setup_proxy(self):
         """Setup the proxy settings if needed."""
         # QtNetwork knows how to use the system settings on both Win and Mac
         if sys.platform.startswith("linux"):
-            self.proxy_factory = WebClientProxyFactory()
-            self.nam.setProxyFactory(self.proxy_factory)
+            settings = gsettings.get_proxy_settings()
+            enabled = len(settings) > 0
+            if enabled and WebClient.proxy_instance is None:
+                proxy = build_proxy(settings)
+                QNetworkProxy.setApplicationProxy(proxy)
+                WebClient.proxy_instance = proxy
+            elif enabled and WebClient.proxy_instance:
+                logger.info("Proxy already in use.")
+            else:
+                logger.info("Proxy is disabled.")
         else:
             QNetworkProxyFactory.setUseSystemConfiguration(True)
 
+    def handle_proxy_auth(self, proxy, authenticator):
+        """Proxy authentication is required."""
+        logger.info("auth_required %r, %r", self.proxy_username,
+                                            proxy.hostName())
+        if (self.proxy_username is not None and
+                self.proxy_username != str(authenticator.user())):
+            authenticator.setUser(self.proxy_username)
+            WebClient.proxy_instance.setUser(self.proxy_username)
+        if (self.proxy_password is not None and
+                self.proxy_password != str(authenticator.password())):
+            authenticator.setPassword(self.proxy_password)
+            WebClient.proxy_instance.setPassword(self.proxy_password)
+
     def _perform_request(self, request, method, post_buffer):
         """Return a deferred that will be fired with a Response object."""
         d = defer.Deferred()
@@ -158,11 +126,6 @@
             reply = self.nam.head(request)
         else:
             reply = self.nam.sendCustomRequest(request, method, post_buffer)
-        # deal with auth errors in case we get
-        # ProxyAuthenticationRequiredError
-        scheme = str(request.url().scheme())
-        d.addErrback(self._handle_proxy_authentication, scheme, method, reply,
-                     post_buffer)
         self.replies[reply] = d
         return d
 
@@ -189,7 +152,20 @@
 
         post_buffer = QBuffer()
         post_buffer.setData(post_content)
-        result = yield self._perform_request(request, method, post_buffer)
+        try:
+            result = yield self._perform_request(request, method, post_buffer)
+        except ProxyUnauthorizedError, e:
+            app_proxy = QNetworkProxy.applicationProxy()
+            proxy_host = app_proxy.hostName() if app_proxy else "proxy server"
+            got_creds = yield self.request_proxy_auth_credentials(
+                                            proxy_host, self.proxy_retry)
+            if got_creds:
+                self.proxy_retry = True
+                result = yield self.request(iri, method, extra_headers,
+                                            oauth_credentials, post_content)
+            else:
+                excp = WebClientError('Proxy creds needed.', e)
+                defer.returnValue(excp)
         defer.returnValue(result)
 
     def _handle_authentication(self, reply, authenticator):
@@ -199,36 +175,6 @@
         if authenticator.password() != self.password:
             authenticator.setPassword(self.password)
 
-    @defer.inlineCallbacks
-    def _handle_proxy_authentication(self, failure, scheme, method, reply,
-            post_buffer=None, retry=False):
-        """The reply needs authentication."""
-        failure.trap(ProxyUnauthorizedError)
-        current_proxy = self.proxy_factory.find_proxy_for_scheme(scheme)
-        host = unicode(current_proxy.hostName()).encode('utf8')
-        port = current_proxy.port()
-        settings = dict(host=host, port=port)
-        # we have to auth the request and retry it
-        if not retry and self.proxy_username and self.proxy_password:
-            settings.update({'username': self.proxy_username,
-                             'password': self.proxy_password})
-        if retry or self.proxy_username is None or self.proxy_password is None:
-            got_creds = yield self.request_proxy_auth_credentials(host, retry)
-            if got_creds:
-                logger.debug('Got proxy credentials from user.')
-                settings.update({'username': self.proxy_username,
-                                 'password': self.proxy_password})
-        if (settings and 'username' in settings
-                and 'password' in settings):
-            request = reply.request()
-            scheme = str(request.url().scheme())
-            self.proxy_factory.update_proxy(scheme, settings)
-            # let's retry using the request
-            result = yield self._perform_request(request, method, post_buffer)
-            defer.returnValue(result)
-        else:
-            defer.returnValue(ProxyUnauthorizedError())
-
     def _handle_finished(self, reply):
         """The reply has finished processing."""
         assert reply in self.replies
@@ -282,9 +228,12 @@
         if trust_cert:
             reply.ignoreSslErrors()
 
-    def force_use_proxy(self, settings):
+    def force_use_proxy(self, https_settings):
         """Setup this webclient to use the given proxy settings."""
-        self.proxy_factory.force_use_proxy(settings)
+        settings = {"https": https_settings}
+        proxy = build_proxy(settings)
+        QNetworkProxy.setApplicationProxy(proxy)
+        WebClient.proxy_instance = proxy
 
     def shutdown(self):
         """Shut down all pending requests (if possible)."""

=== modified file 'ubuntu_sso/utils/webclient/tests/test_webclient.py'
--- ubuntu_sso/utils/webclient/tests/test_webclient.py	2012-03-19 16:34:27 +0000
+++ ubuntu_sso/utils/webclient/tests/test_webclient.py	2012-03-19 23:59:18 +0000
@@ -452,6 +452,8 @@
 class BasicProxyTestCase(SquidTestCase):
     """Test that the proxy works at all."""
 
+    timeout = 3
+
     @defer.inlineCallbacks
     def setUp(self):
         yield super(BasicProxyTestCase, self).setUp()
@@ -595,14 +597,6 @@
         test_auth_proxy_is_requested_creds_bad_details_everywhere.skip = reason
         test_auth_proxy_is_requested_user_cancels.skip = reason
 
-    if WEBCLIENT_MODULE_NAME.endswith(".qtnetwork"):
-        reason = 'QNetworkAccessManager is buggy.'
-        test_auth_proxy_is_used_creds_requested.skip = reason
-        test_auth_proxy_is_requested_creds_bad_details.skip = reason
-        test_auth_proxy_is_requested_creds_bad_details_user.skip = reason
-        test_auth_proxy_is_requested_creds_bad_details_everywhere.skip = reason
-        test_auth_proxy_is_requested_user_cancels.skip = reason
-
 
 class HeaderDictTestCase(TestCase):
     """Tests for the case insensitive header dictionary."""
@@ -1003,6 +997,13 @@
         test_https_request.skip = reason
         test_http_request.skip = reason
 
+    if WEBCLIENT_MODULE_NAME.endswith(".qtnetwork"):
+        reason = ('Updating proxy settings is not well support due to bug'
+                  ' QTBUG-14850: https://bugreports.qt-project.org/'
+                  'browse/QTBUG-14850')
+        test_https_request.skip = reason
+        test_http_request.skip = reason
+
 
 class SSLTestCase(BaseSSLTestCase):
     """Test error handling when dealing with ssl."""