Added OAuth2 authentication to apply_issue

rietveld.py

Index: rietveld.py
diff --git a/rietveld.py b/rietveld.py
index aad998e9c3e49d922ca38317c0e9c1381d88885a..43db1d1041f3f9421f6bd0b7ea64799db467b711 100644
--- a/rietveld.py
+++ b/rietveld.py
@@ -20,11 +20,15 @@ import logging
 import re
 import ssl
 import time
+import urllib
 import urllib2
 
-from third_party import upload
 import patch
 
+from third_party import upload
+from third_party.oauth2client.client import SignedJwtAssertionCredentials
+from third_party import httplib2
+
 # Hack out upload logging.info()
 upload.logging = logging.getLogger('upload')
 # Mac pylint choke on this line.
@@ -39,8 +43,6 @@ class Rietveld(object):
     # It happens when the presubmit check is ran out of process, the cookie
     # needed to be recreated from the credentials. Instead, it should pass the
     # email and the cookie.
-    self.email = email
-    self.password = password
     if email and password:
       get_creds = lambda: (email, password)
       self.rpc_server = upload.HttpRpcServer(
@@ -438,6 +440,131 @@ class Rietveld(object):
   Send = get
 
 
+class OAuthRpcServer():

[M-A Ruel, 2014/03/13 01:31:27]

(object)

[pgervais, 2014/03/13 22:38:38]

Good catch, thanks.

+  def __init__(self,
+               host,
+               client_id,
+               client_private_key,
+               private_key_password='notasecret',
+               user_agent=None,
+               timeout=None,
+               extra_headers=None):
+    """Wrapper around httplib2.Http() that handles authentication.
+
+    client_id: client id for service account
+    client_private_key: encrypted private key, as a string
+    private_key_password: password used to decrypt the private key
+    """
+
+    # Enforce https
+    host_parts = host.split('://')

[M-A Ruel, 2014/03/13 01:31:27]

that's unorthodox.

[Vadim Sh., 2014/03/13 18:28:42]

Yeah.. Use urlparse module.

[pgervais, 2014/03/13 22:38:38]

That's better indeed. But calling split() does exactly what is necessary :-)

+    if host_parts[0] == 'https':  # fine
+      self.host = host
+
+    elif len(host_parts) == 1:   # no protocol specified
+      self.host = 'https://' + host
+
+    elif len(host_parts) == 2 and host_parts[0] == 'http':
+      upload.logging.warning('Changing protocol to https')
+      self.host = 'https://' + host_parts[1]
+
+    else:
+      msg = 'Invalid url provided: %s' % host
+      upload.logging.error(msg)
+      raise ValueError(msg)
+
+    if self.host.endswith('/'):

[Vadim Sh., 2014/03/13 18:28:42]

self.host.rstrip('/')

[pgervais, 2014/03/13 22:38:38]

Done.

+      self.host = self.host[:-1]
+
+    self.extra_headers = extra_headers or {}
+    creds = SignedJwtAssertionCredentials(
+      client_id,
+      client_private_key,
+      'https://www.googleapis.com/auth/userinfo.email',
+      private_key_password,
+      user_agent=user_agent)
+
+    http = httplib2.Http(timeout=timeout)
+    self._http = creds.authorize(http)

[Vadim Sh., 2014/03/13 18:28:42]

It makes the network call here, right? How apply_issue fails if private key is
invalid?

[pgervais, 2014/03/13 22:38:38]

This only decorates the http object, no network access is performed here. It
could fail only line 533.

But the question is valid. I have to double-check everything before actually
committing that.

+

[M-A Ruel, 2014/03/13 01:31:27]

remove one line

[pgervais, 2014/03/13 22:38:38]

Done.

+
+  def Send(self,
+           request_path,
+           payload=None,
+           content_type="application/octet-stream",

[M-A Ruel, 2014/03/13 01:31:27]

use single quote throughout.

[pgervais, 2014/03/13 22:38:38]

I don't know why, but I find it horribly difficult to force myself to use single
quotes only. And double quotes are actually more difficult to get on a qwerty
layout!

+           timeout=None,
+           extra_headers=None,
+           **kwargs):
+    """Send a POST or GET request to the server.
+
+    Args:
+    request_path: path on the server to hit. This is concatenated with the

[M-A Ruel, 2014/03/13 01:31:27]

Align +2

[pgervais, 2014/03/13 22:38:38]

Done.

+      value of 'host' provided to the constructor.
+    payload: request is a POST if not None, GET otherwise
+    timeout: in seconds
+    extra_headers: (dict)
+    """
+    # This method signature should match upload.py:AbstractRpcServer.Send()
+    method = 'GET'
+
+    if extra_headers:
+      headers = self.extra_headers.copy()
+      headers.update(headers)

[M-A Ruel, 2014/03/13 01:31:27]

ugh?

[pgervais, 2014/03/13 22:38:38]

It took me some time to recall what I meant here :-). 

+    else:
+      headers = self.extra_headers

[M-A Ruel, 2014/03/13 01:31:27]

I'd do the copy unconditionally for simplicity; it's also shorter in code:

headers = self.extra_headers.copy()
headers.update(extra_headers or {})

[pgervais, 2014/03/13 22:38:38]

Done.

+
+    if payload is not None:
+      method = 'POST'
+      headers['Content-Type'] = content_type
+      raise NotImplementedError('POST requests are not yet supported.')
+

[M-A Ruel, 2014/03/13 01:31:27]

never 2 empty lines inside a function

[pgervais, 2014/03/13 22:38:38]

Done.

+
+    prev_timeout = self._http.timeout
+    try:
+      if timeout:
+        self._http.timeout = timeout

[M-A Ruel, 2014/03/13 01:31:27]

That's a tad surprising.

+      # TODO(pgervais) implement some kind of retry mechanism (see upload.py).
+      url = self.host + request_path
+      print('Trying URL: ' + url)

[M-A Ruel, 2014/03/13 01:31:27]

Remove.

+      args = dict(kwargs)

[M-A Ruel, 2014/03/13 01:31:27]

It's a dict already.

[pgervais, 2014/03/13 22:38:38]

I copy-pasted part of this code from upload.py, and that very line is in there.
There might have been a reason for it at some point...

Fixed.

+      if args:
+        url += "?" + urllib.urlencode(args)
+
+      ret = self._http.request(url,
+                               method=method,
+                               body=payload,
+                               headers=headers)
+      return ret[1]
+
+    finally:
+      self._http.timeout = prev_timeout

[M-A Ruel, 2014/03/13 01:31:27]

Not a fan.

[pgervais, 2014/03/13 22:38:38]

I have no strong opinion on that matter. I'm fine with any solution you think
would be best.

+
+
+class OAuthRietveld(Rietveld):

[M-A Ruel, 2014/03/13 01:31:27]

But it's really JwtOAuth2Rietveld. Vadim can confirm his preferred semantic.

[Vadim Sh., 2014/03/13 18:28:42]

I don't have preferred semantic, but I agree that JwtOAuth2Rietveld is more
fitting name.

[pgervais, 2014/03/13 22:38:38]

Done.

+  """Access to Rietveld using OAuth authentication.
+
+  This class is supposed to be used only by bots, since this kind of
+  access is restricted to service accounts.
+  """
+  # The parent__init__ is not called on purpose.
+  def __init__(self,  # pylint: disable=W0231

[Vadim Sh., 2014/03/13 18:28:42]

Put pylint disable after ':'.

[pgervais, 2014/03/13 22:38:38]

Done.

+               url,
+               client_id,
+               client_private_key_file,
+               private_key_password='notasecret',
+               extra_headers=None):
+    self.url = url.rstrip('/')
+    with open(client_private_key_file, 'rb') as f:
+      client_private_key = f.read()
+    self.rpc_server = OAuthRpcServer(url,
+                                     client_id,
+                                     client_private_key,
+                                     private_key_password=private_key_password,
+                                     extra_headers=extra_headers or {})
+    self._xsrf_token = None
+    self._xsrf_token_time = None
+
+
 class CachingRietveld(Rietveld):
   """Caches the common queries.