Merge lp:~sil/ubuntuone-client/no-unknown-login-error into lp:ubuntuone-client

[Stuart Langridge]

Remove pycurl from oauthdesktop/auth.py when fetching an oauth token.
Make urllib fail on an invalid SSL certificate.
Do not trap errors; instead, let them propagate up to the top level so we can see them.

[Stuart Langridge (sil)]

Remove pycurl from oauthdesktop/auth.py when fetching an oauth token.
Make urllib fail on an invalid SSL certificate.
Do not trap errors; instead, let them propagate up to the top level so we can see them.

[lp:~sil/ubuntuone-client/no-unknown-login-error]

236. By Stuart Langridge on 2009-10-06

actually do not trap errors

237. By Stuart Langridge on 2009-10-06

missed half a sentence out

[Tim Cole (tcole)]

Hmm. Why are we doing the monkeypatching anew every time we auth, and would it be possible to chain to the old implementation in some way rather than overwriting?

review: Approve

[Tim Cole (tcole)]

Hmm. Why are we doing the monkeypatching anew every time we auth, and would it be possible to chain to the old implementation in some way rather than overwriting?

review: Needs Information

[Stuart Langridge (sil)]

> Hmm. Why are we doing the monkeypatching anew every time we auth, and would
> it be possible to chain to the old implementation in some way rather than
> overwriting?

I thought about chaining, but ssl.wrap_socket(sock, A, B, C, D) is not the same as ssl.wrap_socket(ssl.wrap_socket(sock, A, B), C, D), so essentially the whole thing needs replacing anyway.

Good point on not re-patching every time we auth, but I thought that having the code close in location to where it takes effect will make it clearer to someone debugging later on, since monkeypatching is invisible unless you know it's happening.

[Tim Cole (tcole)]

Hm. Ok, it still seems like doing it at import time would make more sense.

review: Approve

[dobey (dobey)]

./ubuntuone/oauthdesktop/auth.py:
    28: 'pycurl' imported but unused
    29: 'StringIO' imported but unused
    189: undefined name 'socket'
    193: undefined name 'ssl'
    194: undefined name 'ssl'
    196: undefined name 'httplib'
    204: undefined name 'urllib'
    210: undefined name 'data'
    215: undefined name 'data'

make: *** [lint] Error 9

review: Needs Fixing

[lp:~sil/ubuntuone-client/no-unknown-login-error]

238. By Stuart Langridge on 2009-10-06

Move monkeypatching to import time; subclass FancyURLopener to re-add data on redirected POSTs

[Stuart Langridge (sil)]

> make: *** [lint] Error 9

Bizarre, half the code wasn't committed. Please try again. Also fixed Tim's issue above.

[dobey (dobey)]

> > make: *** [lint] Error 9
>
> Bizarre, half the code wasn't committed. Please try again. Also fixed Tim's
> issue above.

Still get:

    28: 'StringIO' imported but unused

[lp:~sil/ubuntuone-client/no-unknown-login-error]

239. By Stuart Langridge on 2009-10-07

lint tidyup

[dobey (dobey)]

94 + opener = FancyURLOpenerWithRedirectedPOST()
95 + fp = opener.open(oauth_request.http_url, oauth_request.to_postdata())
96 + data = fp.read()

It would be great if this block of code could be in a try:/except: clause.

except IOError, e:
    if self.callback_error:
        callback_error(str(e))
    else:
        raise e

[Stuart Langridge (sil)]

> 94 + opener = FancyURLOpenerWithRedirectedPOST()
> 95 + fp = opener.open(oauth_request.http_url,
> oauth_request.to_postdata())
> 96 + data = fp.read()
>
> It would be great if this block of code could be in a try:/except: clause.
>
> except IOError, e:
> if self.callback_error:
> callback_error(str(e))
> else:
> raise e

Done.

[lp:~sil/ubuntuone-client/no-unknown-login-error]

240. By Stuart Langridge on 2009-10-07

Call callback_error if available

[dobey (dobey)]

> > 94 + opener = FancyURLOpenerWithRedirectedPOST()
> > 95 + fp = opener.open(oauth_request.http_url,
> > oauth_request.to_postdata())
> > 96 + data = fp.read()
> >
> > It would be great if this block of code could be in a try:/except: clause.
> >
> > except IOError, e:
> > if self.callback_error:
> > callback_error(str(e))
> > else:
> > raise e
>
> Done.

+ callback_error(str(e))

needs to be self.callback_error(str(e))

Sorry! I am dumb sometimes (particularly in the morninsg when not yet fully awake) :)

[lp:~sil/ubuntuone-client/no-unknown-login-error]

241. By Stuart Langridge on 2009-10-07

copy and paste fail

242. By Stuart Langridge on 2009-10-07

if we hit an error and we notify it upstream with callback_error, bail, don't fall through

[dobey (dobey)]

OFFICIAL SEAL OF ME.

review: Approve

=== modified file 'ubuntuone/oauthdesktop/auth.py'
--- ubuntuone/oauthdesktop/auth.py	2009-09-15 15:50:21 +0000
+++ ubuntuone/oauthdesktop/auth.py	2009-10-07 15:12:12 +0000
@@ -25,10 +25,9 @@
 
 import subprocess
 import random
-import pycurl
-import StringIO
 import dbus
 import os
+import socket, ssl, httplib, urllib
 
 import gnomekeyring
 from oauth import oauth
@@ -60,6 +59,40 @@
 NM_STATE_CONNECTED = 3
 NM_STATE_DISCONNECTED = 4
 
+# Monkeypatch httplib so that urllib will fail on invalid certificate
+def _connect_wrapper(self):
+    """Override HTTPSConnection.connect to require certificate checks"""
+    sock = socket.create_connection((self.host, self.port), self.timeout)
+    if self._tunnel_host:
+        self.sock = sock
+        self._tunnel()
+    self.sock = ssl.wrap_socket(sock, self.key_file, self.cert_file,
+        cert_reqs=ssl.CERT_REQUIRED, 
+        ca_certs="/etc/ssl/certs/ca-certificates.crt")
+httplib.HTTPSConnection.connect = _connect_wrapper
+
+class FancyURLOpenerWithRedirectedPOST(urllib.FancyURLopener):
+    """FancyURLopener does not redirect postdata when redirecting POSTs"""
+    def redirect_internal(self, url, fp, errcode, errmsg, headers, data):
+        """Actually perform a redirect"""
+        # All the same as the original, except passing data, below
+        if 'location' in headers:
+            newurl = headers['location']
+        elif 'uri' in headers:
+            newurl = headers['uri']
+        else:
+            return
+        void = fp.read()
+        fp.close()
+        # In case the server sent a relative URL, join with original:
+        newurl = urllib.basejoin(self.type + ":" + url, newurl)
+        
+        # pass data if present when we redirect
+        if data:
+            return self.open(newurl, data)
+        else:
+            return self.open(newurl)
+
 class AuthorisationClient(object):
     """OAuth authorisation client."""
     def __init__(self, realm, request_token_url, user_authorisation_url,
@@ -183,32 +216,25 @@
 
     def make_token_request(self, oauth_request):
         """Perform the given `OAuthRequest` and return the associated token."""
-        # uses pycurl because that will fail on self-signed certs
-
+        
         logger.debug("Making a token request")
-        accum = StringIO.StringIO()
-        c = pycurl.Curl()
-        c.setopt(c.URL, str(oauth_request.http_url)) # no unicode
-        c.setopt(c.WRITEFUNCTION, accum.write)
-        c.setopt(c.POSTFIELDS, oauth_request.to_postdata())
+        # Note that we monkeypatched httplib above to handle invalid certs
+        # Ways this urlopen can fail:
+        # bad certificate
+        #    raises IOError, e.args[1] == SSLError, e.args[1].errno == 1
+        # No such server 
+        #    raises IOError, e.args[1] == SSLError, e.args[1].errno == -2
         try:
-            c.perform()
-            code = c.getinfo(pycurl.HTTP_CODE)
-            if code == 301:
-                # handle permanent redirect
-                url = c.getinfo(pycurl.REDIRECT_URL)
-                logger.debug('Redirected to: %s', str(url))
-                c.setopt(c.URL, str(url)) # no unicode
-                # reset the StringIO
-                accum.truncate(0)
-                # and retry
-                c.perform()
-        except pycurl.error, e:
-            logger.debug("There was some unknown login error '%s'", e)
-            raise UnknownLoginError(e.message)
-        c.close()
-        accum.seek(0)
-        data = accum.read()
+            opener = FancyURLOpenerWithRedirectedPOST()
+            fp = opener.open(oauth_request.http_url, oauth_request.to_postdata())
+            data = fp.read()
+        except IOError, e:
+            if self.callback_error:
+                self.callback_error(str(e))
+                return
+            else:
+                raise e
+        
         # we deliberately trap anything that might go wrong when parsing the
         # token, because we do not want this to explicitly fail
         # pylint: disable-msg=W0702