Added POST capability to oauth Rietveld

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:
@@ skipping 426 matching lines @@
     finally:
       upload.ErrorExit = old_error_exit
 
   # DEPRECATED.
   Send = get
 
 
 class OAuthRpcServer(object):
   def __init__(self,
                host,
-               client_id,
+               client_email,

[ghost stip (do not use), 2014/04/14 23:23:39]

you sure this is always an email?

[pgervais, 2014/04/14 23:59:25]

When I think of it, no. But appengine provides to different kinds of identifier:
an id and an email, and it's always very confusing. Stating that the email
should be used here helps a lot. 

But your point is valid: on other systems than appengine, it might be something
else than an email :-/ There does not seem to be any constraint in the OAuth2
RFC at least.

                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_email: email associated with the 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 = urlparse.urlparse(host)
 
     if host_parts.scheme == 'https':  # fine
       self.host = host
     elif host_parts.scheme == 'http':
       upload.logging.warning('Changing protocol to https')
       self.host = 'https' + host[4:]
     else:
       msg = 'Invalid url provided: %s' % host
       upload.logging.error(msg)
       raise ValueError(msg)
 
     self.host = self.host.rstrip('/')
 
     self.extra_headers = extra_headers or {}
 
     if not oa2client.HAS_OPENSSL:
-      logging.error("Support for OpenSSL hasn't been found, "
+      logging.error("No support for OpenSSL has been found, "
                     "OAuth2 support requires it.")
       logging.error("Installing pyopenssl will probably solve this issue.")
       raise RuntimeError('No OpenSSL support')
     creds = oa2client.SignedJwtAssertionCredentials(
-      client_id,
+      client_email,
       client_private_key,
       'https://www.googleapis.com/auth/userinfo.email',
       private_key_password=private_key_password,
       user_agent=user_agent)
 
     self._http = creds.authorize(httplib2.Http(timeout=timeout))
 
   def Send(self,
            request_path,
            payload=None,
@@ skipping 12 matching lines @@
     """
     # This method signature should match upload.py:AbstractRpcServer.Send()
     method = 'GET'
 
     headers = self.extra_headers.copy()
     headers.update(extra_headers or {})
 
     if payload is not None:
       method = 'POST'
       headers['Content-Type'] = content_type
-      raise NotImplementedError('POST requests are not yet supported.')
 
     prev_timeout = self._http.timeout
     try:
       if timeout:
         self._http.timeout = timeout
       # TODO(pgervais) implement some kind of retry mechanism (see upload.py).
       url = self.host + request_path
       if kwargs:
         url += "?" + urllib.urlencode(kwargs)
 
       ret = self._http.request(url,
                                method=method,
                                body=payload,
                                headers=headers)
-      if not ret[0]['content-location'].startswith(self.host):
+
+      if (method == 'GET'
+          and not ret[0]['content-location'].startswith(self.host)):
         upload.logging.warning('Redirection to host %s detected: '
                                'login may have failed/expired.'
                                % urlparse.urlparse(
                                      ret[0]['content-location']).netloc)
       return ret[1]
 
     finally:
       self._http.timeout = prev_timeout
 
 
 class JwtOAuth2Rietveld(Rietveld):
   """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.
   # pylint: disable=W0231
   def __init__(self,
                url,
-               client_id,
+               client_email,
                client_private_key_file,
                private_key_password=None,
                extra_headers=None):
+
+    # These attributes are accessed by commit queue. Keep them.
+    self.email = client_email
+    self.private_key_file = client_private_key_file
+
     if private_key_password is None:  # '' means 'empty password'
       private_key_password = 'notasecret'
 
     self.url = url.rstrip('/')
+    bot_url = self.url
+    if not bot_url.endswith('/bots'):

[ghost stip (do not use), 2014/04/14 23:23:39]

why not just specify /bots originally?

[pgervais, 2014/04/14 23:59:25]

In practice, I found it more natural to specify the root URL, because it does
not depend on the authentication method. Adding /bots here also makes it easier
to change that prefix later on. 

Adding '/bots' when it's missing is rather bad practice. As a reviewer I think I
would complain about it :-) Maybe I'll drop it and prey that doesn't break any
other code that I've written.

+      bot_url += '/bots'
+
     with open(client_private_key_file, 'rb') as f:
       client_private_key = f.read()
-    self.rpc_server = OAuthRpcServer(url,
-                                     client_id,
+    logging.info('Using OAuth login: %s' % client_email)
+    self.rpc_server = OAuthRpcServer(bot_url,
+                                     client_email,
                                      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.
 
@@ skipping 119 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))