Added OAuth2 authentication to apply_issue

rietveld.py

 # coding: utf-8
 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 """Defines class Rietveld to easily access a rietveld instance.
 
 Security implications:
 
 The following hypothesis are made:
 - Rietveld enforces:
   - Nobody else than issue owner can upload a patch set
   - Verifies the issue owner credentials when creating new issues
   - A issue owner can't change once the issue is created
   - A patch set cannot be modified
 """
 
 import copy
 import json
 import logging
 import re
 import ssl
 import time
+import urllib
 import urllib2
 
+import patch
+
 from third_party import upload
-import patch
+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.
 upload.logging.setLevel(logging.WARNING)  # pylint: disable=E1103
 
 
 class Rietveld(object):
   """Accesses rietveld."""
   def __init__(self, url, email, password, extra_headers=None):
     self.url = url.rstrip('/')
     # TODO(maruel): It's not awesome but maybe necessary to retrieve the value.
     # 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(
             self.url,
             get_creds,
             extra_headers=extra_headers or {})
     else:
       if email == '':
         # If email is given as an empty string, then assume we want to make
         # requests that do not need authentication.  Bypass authentication by
@@ skipping 377 matching lines @@
             raise
         # If reaching this line, loop again. Uses a small backoff.
         time.sleep(1+maxtries*2)
     finally:
       upload.ErrorExit = old_error_exit
 
   # DEPRECATED.
   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.
 
   Not to be used in long-standing processes, like the commit queue.
   """
   def __init__(self, *args, **kwargs):
     super(CachingRietveld, self).__init__(*args, **kwargs)
     self._cache = {}
 
   def _lookup(self, function_name, args, update):
@@ skipping 108 matching lines @@
   def trigger_try_jobs(  # pylint:disable=R0201
       self, issue, patchset, reason, clobber, revision, builders_and_tests,
       master=None):
     logging.info('ReadOnlyRietveld: triggering try jobs %r for issue %d' %
         (builders_and_tests, issue))
 
   def trigger_distributed_try_jobs(  # pylint:disable=R0201
       self, issue, patchset, reason, clobber, revision, masters):
     logging.info('ReadOnlyRietveld: triggering try jobs %r for issue %d' %
         (masters, issue))