Extract authentication options handling into a separate function.

It is done in preparation for switching to OAuth2 as default (and only)
authentication method. Having all auth options handled by the same code makes it
easier to gradually add OAuth2 support.

As part of this, some options that would no longer work with OAuth2 (and that
are not being used from anywhere now, as far as I can tell) are removed:
  * Passing account password for authentication via command line.
  * Overriding 'Host' header when making requests to Rietveld (won't work with
    SSL anyway).
  * --account_type option (seems to be ClientLogin specific).

R=maruel@chromium.org
BUG=356813

Review URL: https://codereview.chromium.org/1075723002

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@294746 0039d316-1c4b-4281-b951-d872f2087c98

apply_issue.py

@@ -18,6 +18,7 @@ import urllib2
 import breakpad  # pylint: disable=W0611
 
 import annotated_gclient
+import auth
 import checkout
 import fix_encoding
 import gclient_utils
@@ -55,14 +56,11 @@ def main():
       '-E', '--email-file',
       help='File containing the email address to access rietveld. '
            'If not specified, anonymous access will be used.')
-  parser.add_option(
-      '-w', '--password',
-      help='Password for email addressed. Use - to read password from stdin. '
-           'if -k is provided, this is the private key file password.')
   parser.add_option(
       '-k', '--private-key-file',
       help='Path to file containing a private key in p12 format for OAuth2 '
-           'authentication. Use -w to provide the decrypting password, if any.')
+           'authentication with "notasecret" password (as generated by Google '
+           'Cloud Console).')
   parser.add_option(
       '-i', '--issue', type='int', help='Rietveld issue number')
   parser.add_option(
@@ -92,13 +90,14 @@ def main():
                     help='Don\'t patch specified file(s).')
   parser.add_option('-d', '--ignore_deps', action='store_true',
                     help='Don\'t run gclient sync on DEPS changes.')
+
+  auth.add_auth_options(parser)
   options, args = parser.parse_args()
+  auth_config = auth.extract_auth_config_from_options(options)
 
   if options.whitelist and options.blacklist:
     parser.error('Cannot specify both --whitelist and --blacklist')
 
-  if options.password and options.private_key_file:
-    parser.error('-k and -w options are incompatible')
   if options.email and options.email_file:
     parser.error('-e and -E options are incompatible')
 
@@ -121,10 +120,6 @@ def main():
 
   options.revision_mapping = json.loads(options.revision_mapping)
 
-  if options.password == '-':
-    print('Reading password')
-    options.password = sys.stdin.readline().strip()
-
   # read email if needed
   if options.email_file:
     if not os.path.exists(options.email_file):
@@ -138,11 +133,11 @@ def main():
     # OAuth2 authentication
     obj = rietveld.JwtOAuth2Rietveld(options.server,
                                      options.email,
-                                     options.private_key_file,
-                                     private_key_password=options.password)
+                                     options.private_key_file)
     properties = obj.get_issue_properties(options.issue, False)
   else:
-    obj = rietveld.Rietveld(options.server, '', None)
+    # Passing None as auth_config disables authentication.
+    obj = rietveld.Rietveld(options.server, None)
     properties = None
     # Bad except clauses order (HTTPError is an ancestor class of
     # ClientLoginError)
@@ -156,26 +151,16 @@ def main():
         exit('FAIL: Login detected -- is issue private?')
       # TODO(maruel): A few 'Invalid username or password.' are printed first,
       # we should get rid of those.
-    except rietveld.upload.ClientLoginError, e:
+    except rietveld.upload.ClientLoginError as e:
       # Fine, we'll do proper authentication.
       pass
     if properties is None:
-      if options.email is not None:
-        obj = rietveld.Rietveld(options.server, options.email, options.password)
-        try:
-          properties = obj.get_issue_properties(options.issue, False)
-        except rietveld.upload.ClientLoginError, e:
-          if sys.stdout.closed:
-            print('Accessing the issue requires proper credentials.')
-            return 1
-      else:
-        print('Accessing the issue requires login.')
-        obj = rietveld.Rietveld(options.server, None, None)
-        try:
-          properties = obj.get_issue_properties(options.issue, False)
-        except rietveld.upload.ClientLoginError, e:
-          print('Accessing the issue requires proper credentials.')
-          return 1
+      obj = rietveld.Rietveld(options.server, auth_config, options.email)
+      try:
+        properties = obj.get_issue_properties(options.issue, False)
+      except rietveld.upload.ClientLoginError as e:
+        print('Accessing the issue requires proper credentials.')
+        return 1
 
   if not options.patchset:
     options.patchset = properties['patchsets'][-1]
@@ -184,7 +169,7 @@ def main():
   print('Downloading the patch.')
   try:
     patchset = obj.get_patch(options.issue, options.patchset)
-  except urllib2.HTTPError, e:
+  except urllib2.HTTPError as e:
     print(
         'Failed to fetch the patch for issue %d, patchset %d.\n'
         'Try visiting %s/%d') % (
@@ -222,7 +207,7 @@ def main():
   print('\nApplying the patch.')
   try:
     scm_obj.apply_patch(patchset, verbose=True)
-  except checkout.PatchApplicationFailed, e:
+  except checkout.PatchApplicationFailed as e:
     print(str(e))
     print('CWD=%s' % os.getcwd())
     print('Checkout path=%s' % scm_obj.project_path)

auth.py

+# Copyright (c) 2015 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.
+
+"""Authentication related functions."""
+
+import collections
+import optparse
+
+
+# Authentication configuration extracted from command line options.
+# See doc string for 'make_auth_config' for meaning of fields.
+AuthConfig = collections.namedtuple('AuthConfig', [
+    'use_oauth2', # deprecated, will be always True
+    'save_cookies', # deprecated, will be removed
+    'use_local_webserver',
+    'webserver_port',
+])
+
+
+def make_auth_config(
+    use_oauth2=None,
+    save_cookies=None,
+    use_local_webserver=None,
+    webserver_port=None):
+  """Returns new instance of AuthConfig.
+
+  If some config option is None, it will be set to a reasonable default value.
+  This function also acts as an authoritative place for default values of
+  corresponding command line options.
+  """
+  default = lambda val, d: val if val is not None else d
+  return AuthConfig(
+      default(use_oauth2, False),
+      default(save_cookies, True),
+      default(use_local_webserver, True),
+      default(webserver_port, 8090))
+
+
+def add_auth_options(parser):
+  """Appends OAuth related options to OptionParser."""
+  default_config = make_auth_config()
+  parser.auth_group = optparse.OptionGroup(parser, 'Auth options')
+  parser.auth_group.add_option(
+      '--oauth2',
+      action='store_true',
+      dest='use_oauth2',
+      default=default_config.use_oauth2,
+      help='Use OAuth 2.0 instead of a password.')
+  parser.auth_group.add_option(
+      '--no-oauth2',
+      action='store_false',
+      dest='use_oauth2',
+      default=default_config.use_oauth2,
+      help='Use password instead of OAuth 2.0.')
+  parser.auth_group.add_option(
+      '--no-cookies',
+      action='store_false',
+      dest='save_cookies',
+      default=default_config.save_cookies,
+      help='Do not save authentication cookies to local disk.')
+  parser.auth_group.add_option(
+      '--auth-no-local-webserver',
+      action='store_false',
+      dest='use_local_webserver',
+      default=default_config.use_local_webserver,
+      help='Do not run a local web server when performing OAuth2 login flow.')
+  parser.auth_group.add_option(
+      '--auth-host-port',
+      type=int,
+      default=default_config.webserver_port,
+      help='Port a local web server should listen on. Used only if '
+          '--auth-no-local-webserver is not set. [default: %default]')
+  parser.add_option_group(parser.auth_group)
+
+
+def extract_auth_config_from_options(options):
+  """Given OptionParser parsed options, extracts AuthConfig from it.
+
+  OptionParser should be populated with auth options by 'add_auth_options'.
+  """
+  return make_auth_config(
+      use_oauth2=options.use_oauth2,
+      save_cookies=False if options.use_oauth2 else options.save_cookies,
+      use_local_webserver=options.use_local_webserver,
+      webserver_port=options.auth_host_port)
+
+
+def auth_config_to_command_options(auth_config):
+  """AuthConfig -> list of strings with command line options."""
+  if not auth_config:
+    return []
+  opts = ['--oauth2' if auth_config.use_oauth2 else '--no-oauth2']
+  if not auth_config.save_cookies:
+    opts.append('--no-cookies')
+  if not auth_config.use_local_webserver:
+    opts.append('--auth-no-local-webserver')
+  opts.extend(['--auth-host-port', str(auth_config.webserver_port)])
+  return opts

commit_queue.py

@@ -17,6 +17,7 @@ import urllib2
 
 import breakpad  # pylint: disable=W0611
 
+import auth
 import fix_encoding
 import rietveld
 
@@ -36,9 +37,10 @@ def need_issue(fn):
 
     def new_parse_args(args=None, values=None):
       options, args = old_parse_args(args, values)
+      auth_config = auth.extract_auth_config_from_options(options)
       if not options.issue:
         parser.error('Require --issue')
-      obj = rietveld.Rietveld(options.server, options.user, None)
+      obj = rietveld.Rietveld(options.server, auth_config, options.user)
       return options, args, obj
 
     parser.parse_args = new_parse_args
@@ -59,6 +61,7 @@ def need_issue(fn):
         metavar='S',
         default='http://codereview.chromium.org',
         help='Rietveld server, default: %default')
+    auth.add_auth_options(parser)
 
     # Call the original function with the modified parser.
     return fn(parser, args, *extra_args, **kwargs)

gcl.py

@@ -23,6 +23,7 @@ import urllib2
 import breakpad  # pylint: disable=W0611
 
 
+import auth
 import fix_encoding
 import gclient_utils
 import git_cl
@@ -351,7 +352,10 @@ class ChangeInfo(object):
     if not self._rpc_server:
       if not self.rietveld:
         ErrorExit(CODEREVIEW_SETTINGS_FILE_NOT_FOUND)
-      self._rpc_server = rietveld.CachingRietveld(self.rietveld, None, None)
+      # TODO(vadimsh): glc.py should be deleted soon. Do not bother much about
+      # authentication options and always use defaults.
+      self._rpc_server = rietveld.CachingRietveld(
+          self.rietveld, auth.make_auth_config())
     return self._rpc_server
 
   def CloseIssue(self):
@@ -1465,6 +1469,10 @@ def main(argv):
         '\nYour python version %s is unsupported, please upgrade.\n' %
         sys.version.split(' ', 1)[0])
     return 2
+
+  sys.stderr.write('Warning: gcl is going away soon. Get off subversion!\n')
+  sys.stderr.write('See http://crbug.com/475321 for more details.\n')
+
   if not argv:
     argv = ['help']
   command = Command(argv[0])

git_cherry_pick_upload.py

@@ -5,23 +5,26 @@
 
 """Upload a cherry pick CL to rietveld."""
 
-import argparse
 import md5
+import optparse
 import subprocess2
 import sys
 
+import auth
+
 from git_cl import Changelist
 from git_common import config, run
 from third_party.upload import EncodeMultipartFormData, GitVCS
 from rietveld import Rietveld
 
 
-def cherry_pick(target_branch, commit):
+def cherry_pick(target_branch, commit, auth_config):
   """Attempt to upload a cherry pick CL to rietveld.
 
   Args:
     target_branch: The branch to cherry pick onto.
     commit: The git hash of the commit to cherry pick.
+    auth_config: auth.AuthConfig object with authentication configuration.
   """
   author = config('user.email')
 
@@ -48,7 +51,7 @@ def cherry_pick(target_branch, commit):
           run('diff', parent, commit))),
   ])
 
-  rietveld = Rietveld(config('rietveld.server'), author, None)
+  rietveld = Rietveld(config('rietveld.server'), auth_config, author)
   # pylint: disable=W0212
   output = rietveld._send(
     '/upload',
@@ -124,21 +127,23 @@ def cherry_pick(target_branch, commit):
 
 
 def main():
-  parser = argparse.ArgumentParser()
-  parser.add_argument(
+  parser = optparse.OptionParser(
+      usage='usage: %prog --branch <branch> <commit>')
+  parser.add_option(
       '--branch',
       '-b',
       help='The upstream branch to cherry pick to.',
-      metavar='<branch>',
-      required=True,
-  )
-  parser.add_argument(
-      'commit',
-      help='SHA to cherry pick.',
-      metavar='<commit>',
-  )
-  args = parser.parse_args()
-  cherry_pick(args.branch, args.commit)
+      metavar='<branch>')
+  auth.add_auth_options(parser)
+  options, args = parser.parse_args()
+  auth_config = auth.extract_auth_config_from_options
+
+  if not options.branch:
+    parser.error('--branch is required')
+  if len(args) != 1:
+    parser.error('Expecting single argument <commit>')
+
+  cherry_pick(options.branch, args[0], auth_config)
   return 0
 
 

git_cl.py

@@ -34,6 +34,7 @@ except ImportError:
 
 from third_party import colorama
 from third_party import upload
+import auth
 import breakpad  # pylint: disable=W0611
 import clang_format
 import dart_format
@@ -484,7 +485,7 @@ def ShortBranchName(branch):
 
 
 class Changelist(object):
-  def __init__(self, branchref=None, issue=None):
+  def __init__(self, branchref=None, issue=None, auth_config=None):
     # Poke settings so we get the "configure your server" message if necessary.
     global settings
     if not settings:
@@ -504,11 +505,16 @@ class Changelist(object):
     self.description = None
     self.lookedup_patchset = False
     self.patchset = None
-    self._rpc_server = None
     self.cc = None
     self.watchers = ()
-    self._remote = None
+    self._auth_config = auth_config
     self._props = None
+    self._remote = None
+    self._rpc_server = None
+
+  @property
+  def auth_config(self):
+    return self._auth_config
 
   def GetCCList(self):
     """Return the users cc'd on this CL.
@@ -971,7 +977,8 @@ or verify this branch is set up to track another (via the --track argument to
     """
     if not self._rpc_server:
       self._rpc_server = rietveld.CachingRietveld(
-          self.GetRietveldServer(), None, None)
+          self.GetRietveldServer(),
+          self._auth_config or auth.make_auth_config())
     return self._rpc_server
 
   def _IssueSetting(self):
@@ -1328,9 +1335,9 @@ def color_for_status(status):
     'error': Fore.WHITE,
   }.get(status, Fore.WHITE)
 
-def fetch_cl_status(b):
+def fetch_cl_status(b, auth_config=None):
   """Fetches information for an issue and returns (branch, issue, color)."""
-  c = Changelist(branchref=b)
+  c = Changelist(branchref=b, auth_config=auth_config)
   i = c.GetIssueURL()
   status = c.GetStatus()
   color = color_for_status(status)
@@ -1341,7 +1348,8 @@ def fetch_cl_status(b):
 
   return (b, i, color)
 
-def get_cl_statuses(branches, fine_grained, max_processes=None):
+def get_cl_statuses(
+    branches, fine_grained, max_processes=None, auth_config=None):
   """Returns a blocking iterable of (branch, issue, color) for given branches.
 
   If fine_grained is true, this will fetch CL statuses from the server.
@@ -1358,19 +1366,20 @@ def get_cl_statuses(branches, fine_grained, max_processes=None):
     # Process one branch synchronously to work through authentication, then
     # spawn processes to process all the other branches in parallel.
     if branches:
-      yield fetch_cl_status(branches[0])
+      fetch = lambda branch: fetch_cl_status(branch, auth_config=auth_config)
+      yield fetch(branches[0])
 
       branches_to_fetch = branches[1:]
       pool = ThreadPool(
           min(max_processes, len(branches_to_fetch))
               if max_processes is not None
               else len(branches_to_fetch))
-      for x in pool.imap_unordered(fetch_cl_status, branches_to_fetch):
+      for x in pool.imap_unordered(fetch, branches_to_fetch):
         yield x
   else:
     # Do not use GetApprovingReviewers(), since it requires an HTTP request.
     for b in branches:
-      c = Changelist(branchref=b)
+      c = Changelist(branchref=b, auth_config=auth_config)
       url = c.GetIssueURL()
       yield (b, url, Fore.BLUE if url else Fore.WHITE)
 
@@ -1394,12 +1403,15 @@ def CMDstatus(parser, args):
   parser.add_option(
       '-j', '--maxjobs', action='store', type=int,
       help='The maximum number of jobs to use when retrieving review status')
-  (options, args) = parser.parse_args(args)
+
+  auth.add_auth_options(parser)
+  options, args = parser.parse_args(args)
   if args:
     parser.error('Unsupported args: %s' % args)
+  auth_config = auth.extract_auth_config_from_options(options)
 
   if options.field:
-    cl = Changelist()
+    cl = Changelist(auth_config=auth_config)
     if options.field.startswith('desc'):
       print cl.GetDescription()
     elif options.field == 'id':
@@ -1421,13 +1433,16 @@ def CMDstatus(parser, args):
     print('No local branch found.')
     return 0
 
-  changes = (Changelist(branchref=b) for b in branches.splitlines())
+  changes = (
+      Changelist(branchref=b, auth_config=auth_config)
+      for b in branches.splitlines())
   branches = [c.GetBranch() for c in changes]
   alignment = max(5, max(len(b) for b in branches))
   print 'Branches associated with reviews:'
   output = get_cl_statuses(branches,
                            fine_grained=not options.fast,
-                           max_processes=options.maxjobs)
+                           max_processes=options.maxjobs,
+                           auth_config=auth_config)
 
   branch_statuses = {}
   alignment = max(5, max(len(ShortBranchName(b)) for b in branches))
@@ -1443,7 +1458,7 @@ def CMDstatus(parser, args):
     print '  %*s : %s%s%s' % (
           alignment, ShortBranchName(branch), color, issue, reset)
 
-  cl = Changelist()
+  cl = Changelist(auth_config=auth_config)
   print
   print 'Current branch:',
   if not cl.GetIssue():
@@ -1520,7 +1535,9 @@ def CMDcomments(parser, args):
                     help='comment to add to an issue')
   parser.add_option('-i', dest='issue',
                     help="review issue id (defaults to current issue)")
+  auth.add_auth_options(parser)
   options, args = parser.parse_args(args)
+  auth_config = auth.extract_auth_config_from_options(options)
 
   issue = None
   if options.issue:
@@ -1529,7 +1546,7 @@ def CMDcomments(parser, args):
     except ValueError:
       DieWithError('A review issue id is expected to be a number')
 
-  cl = Changelist(issue=issue)
+  cl = Changelist(issue=issue, auth_config=auth_config)
 
   if options.comment:
     cl.AddComment(options.comment)
@@ -1555,8 +1572,10 @@ def CMDcomments(parser, args):
 
 def CMDdescription(parser, args):
   """Brings up the editor for the current CL's description."""
-  parser.parse_args(args)
-  cl = Changelist()
+  auth.add_auth_options(parser)
+  options, _ = parser.parse_args(args)
+  auth_config = auth.extract_auth_config_from_options(options)
+  cl = Changelist(auth_config=auth_config)
   if not cl.GetIssue():
     DieWithError('This branch has no associated changelist.')
   description = ChangeDescription(cl.GetDescription())
@@ -1584,7 +1603,9 @@ def CMDlint(parser, args):
   """Runs cpplint on the current changelist."""
   parser.add_option('--filter', action='append', metavar='-x,+y',
                     help='Comma-separated list of cpplint\'s category-filters')
-  (options, args) = parser.parse_args(args)
+  auth.add_auth_options(parser)
+  options, args = parser.parse_args(args)
+  auth_config = auth.extract_auth_config_from_options(options)
 
   # Access to a protected member _XX of a client class
   # pylint: disable=W0212
@@ -1600,7 +1621,7 @@ def CMDlint(parser, args):
   previous_cwd = os.getcwd()
   os.chdir(settings.GetRoot())
   try:
-    cl = Changelist()
+    cl = Changelist(auth_config=auth_config)
     change = cl.GetChange(cl.GetCommonAncestorWithUpstream(), None)
     files = [f.LocalPath() for f in change.AffectedFiles()]
     if not files:
@@ -1639,13 +1660,15 @@ def CMDpresubmit(parser, args):
                     help='Run upload hook instead of the push/dcommit hook')
   parser.add_option('-f', '--force', action='store_true',
                     help='Run checks even if tree is dirty')
-  (options, args) = parser.parse_args(args)
+  auth.add_auth_options(parser)
+  options, args = parser.parse_args(args)
+  auth_config = auth.extract_auth_config_from_options(options)
 
   if not options.force and git_common.is_dirty_git_tree('presubmit'):
     print 'use --force to check even if tree is dirty.'
     return 1
 
-  cl = Changelist()
+  cl = Changelist(auth_config=auth_config)
   if args:
     base_branch = args[0]
   else:
@@ -1845,6 +1868,7 @@ def RietveldUpload(options, args, cl, change):
   """upload the patch to rietveld."""
   upload_args = ['--assume_yes']  # Don't ask about untracked files.
   upload_args.extend(['--server', cl.GetRietveldServer()])
+  upload_args.extend(auth.auth_config_to_command_options(cl.auth_config))
   if options.emulate_svn_auto_props:
     upload_args.append('--emulate_svn_auto_props')
 
@@ -2016,7 +2040,9 @@ def CMDupload(parser, args):
                          'upload.')
 
   add_git_similarity(parser)
+  auth.add_auth_options(parser)
   (options, args) = parser.parse_args(args)
+  auth_config = auth.extract_auth_config_from_options(options)
 
   if git_common.is_dirty_git_tree('upload'):
     return 1
@@ -2024,7 +2050,7 @@ def CMDupload(parser, args):
   options.reviewers = cleanup_list(options.reviewers)
   options.cc = cleanup_list(options.cc)
 
-  cl = Changelist()
+  cl = Changelist(auth_config=auth_config)
   if args:
     # TODO(ukai): is it ok for gerrit case?
     base_branch = args[0]
@@ -2118,8 +2144,11 @@ def SendUpstream(parser, args, cmd):
                          "description and used as author for git). Should be " +
                          "formatted as 'First Last <email@example.com>'")
   add_git_similarity(parser)
+  auth.add_auth_options(parser)
   (options, args) = parser.parse_args(args)
-  cl = Changelist()
+  auth_config = auth.extract_auth_config_from_options(options)
+
+  cl = Changelist(auth_config=auth_config)
 
   current = cl.GetBranch()
   remote, upstream_branch = cl.FetchUpstreamTuple(cl.GetBranch())
@@ -2517,7 +2546,10 @@ def CMDpatch(parser, args):
                         'attempting a 3-way merge')
   parser.add_option('-n', '--no-commit', action='store_true', dest='nocommit',
                     help="don't commit after patch applies")
+  auth.add_auth_options(parser)
   (options, args) = parser.parse_args(args)
+  auth_config = auth.extract_auth_config_from_options(options)
+
   if len(args) != 1:
     parser.print_help()
     return 1
@@ -2538,10 +2570,10 @@ def CMDpatch(parser, args):
             Changelist().GetUpstreamBranch()])
 
   return PatchIssue(issue_arg, options.reject, options.nocommit,
-                    options.directory)
+                    options.directory, auth_config)
 
 
-def PatchIssue(issue_arg, reject, nocommit, directory):
+def PatchIssue(issue_arg, reject, nocommit, directory, auth_config):
   # There's a "reset --hard" when failing to apply the patch. In order
   # not to destroy users' data, make sure the tree is not dirty here.
   assert(not git_common.is_dirty_git_tree('apply'))
@@ -2549,7 +2581,7 @@ def PatchIssue(issue_arg, reject, nocommit, directory):
   if type(issue_arg) is int or issue_arg.isdigit():
     # Input is an issue id.  Figure out the URL.
     issue = int(issue_arg)
-    cl = Changelist(issue=issue)
+    cl = Changelist(issue=issue, auth_config=auth_config)
     patchset = cl.GetMostRecentPatchset()
     patch_data = cl.GetPatchSetDiff(issue, patchset)
   else:
@@ -2602,7 +2634,7 @@ def PatchIssue(issue_arg, reject, nocommit, directory):
     RunGit(['commit', '-m', ('patch from issue %(i)s at patchset '
                              '%(p)s (http://crrev.com/%(i)s#ps%(p)s)'
                              % {'i': issue, 'p': patchset})])
-    cl = Changelist()
+    cl = Changelist(auth_config=auth_config)
     cl.SetIssue(issue)
     cl.SetPatchset(patchset)
     print "Committed patch locally."
@@ -2722,12 +2754,14 @@ def CMDtry(parser, args):
   group.add_option(
       "-n", "--name", help="Try job name; default to current branch name")
   parser.add_option_group(group)
+  auth.add_auth_options(parser)
   options, args = parser.parse_args(args)
+  auth_config = auth.extract_auth_config_from_options(options)
 
   if args:
     parser.error('Unknown arguments: %s' % args)
 
-  cl = Changelist()
+  cl = Changelist(auth_config=auth_config)
   if not cl.GetIssue():
     parser.error('Need to upload first')
 
@@ -2875,10 +2909,12 @@ def CMDweb(parser, args):
 
 def CMDset_commit(parser, args):
   """Sets the commit bit to trigger the Commit Queue."""
-  _, args = parser.parse_args(args)
+  auth.add_auth_options(parser)
+  options, args = parser.parse_args(args)
+  auth_config = auth.extract_auth_config_from_options(options)
   if args:
     parser.error('Unrecognized args: %s' % ' '.join(args))
-  cl = Changelist()
+  cl = Changelist(auth_config=auth_config)
   props = cl.GetIssueProperties()
   if props.get('private'):
     parser.error('Cannot set commit on private issue')
@@ -2888,10 +2924,12 @@ def CMDset_commit(parser, args):
 
 def CMDset_close(parser, args):
   """Closes the issue."""
-  _, args = parser.parse_args(args)
+  auth.add_auth_options(parser)
+  options, args = parser.parse_args(args)
+  auth_config = auth.extract_auth_config_from_options(options)
   if args:
     parser.error('Unrecognized args: %s' % ' '.join(args))
-  cl = Changelist()
+  cl = Changelist(auth_config=auth_config)
   # Ensure there actually is an issue to close.
   cl.GetDescription()
   cl.CloseIssue()
@@ -2900,7 +2938,11 @@ def CMDset_close(parser, args):
 
 def CMDdiff(parser, args):
   """Shows differences between local tree and last upload."""
-  parser.parse_args(args)
+  auth.add_auth_options(parser)
+  options, args = parser.parse_args(args)
+  auth_config = auth.extract_auth_config_from_options(options)
+  if args:
+    parser.error('Unrecognized args: %s' % ' '.join(args))
 
   # Uncommitted (staged and unstaged) changes will be destroyed by
   # "git reset --hard" if there are merging conflicts in PatchIssue().
@@ -2910,7 +2952,7 @@ def CMDdiff(parser, args):
   if git_common.is_dirty_git_tree('diff'):
     return 1
 
-  cl = Changelist()
+  cl = Changelist(auth_config=auth_config)
   issue = cl.GetIssue()
   branch = cl.GetBranch()
   if not issue:
@@ -2922,7 +2964,7 @@ def CMDdiff(parser, args):
   RunGit(['checkout', '-q', '-b', TMP_BRANCH, base_branch])
   try:
     # Patch in the latest changes from rietveld.
-    rtn = PatchIssue(issue, False, False, None)
+    rtn = PatchIssue(issue, False, False, None, auth_config)
     if rtn != 0:
       return rtn
 
@@ -2942,11 +2984,13 @@ def CMDowners(parser, args):
       '--no-color',
       action='store_true',
       help='Use this option to disable color output')
+  auth.add_auth_options(parser)
   options, args = parser.parse_args(args)
+  auth_config = auth.extract_auth_config_from_options(options)
 
   author = RunGit(['config', 'user.email']).strip() or None
 
-  cl = Changelist()
+  cl = Changelist(auth_config=auth_config)
 
   if args:
     if len(args) > 1:

my_activity.py

@@ -13,6 +13,9 @@ Example:
   - my_activity.py -b 4/5/12 -e 6/7/12 for stats between 4/5/12 and 6/7/12.
 """
 
+# TODO(vadimsh): This script knows too much about ClientLogin and cookies. It
+# will stop to work on ~20 Apr 2015.
+
 # These services typically only provide a created time and a last modified time
 # for each item for general queries. This is not enough to determine if there
 # was activity in a given time period. So, we first query for all things created
@@ -33,6 +36,7 @@ import sys
 import urllib
 import urllib2
 
+import auth
 import gerrit_util
 import rietveld
 from third_party import upload
@@ -267,7 +271,8 @@ class MyActivity(object):
 
 
     email = None if instance['auth'] else ''
-    remote = rietveld.Rietveld('https://' + instance['url'], email, None)
+    auth_config = auth.extract_auth_config_from_options(self.options)
+    remote = rietveld.Rietveld('https://' + instance['url'], auth_config, email)
 
     # See def search() in rietveld.py to see all the filters you can use.
     query_modified_after = None
@@ -780,6 +785,7 @@ def main():
       help='Use markdown-friendly output (overrides --output-format '
            'and --output-format-heading)')
   parser.add_option_group(output_format_group)
+  auth.add_auth_options(parser)
 
   # Remove description formatting
   parser.format_description = (

my_reviews.py

@@ -14,6 +14,7 @@ import optparse
 import os
 import sys
 
+import auth
 import rietveld
 
 
@@ -214,9 +215,10 @@ def print_issue(issue, reviewer, stats):
       ', '.join(sorted(issue['reviewers'])))
 
 
-def print_reviews(reviewer, created_after, created_before, instance_url):
+def print_reviews(
+    reviewer, created_after, created_before, instance_url, auth_config):
   """Prints issues |reviewer| received and potentially reviewed."""
-  remote = rietveld.Rietveld(instance_url, None, None)
+  remote = rietveld.Rietveld(instance_url, auth_config)
 
   # The stats we gather. Feel free to send me a CL to get more stats.
   stats = Stats()
@@ -268,8 +270,9 @@ def print_reviews(reviewer, created_after, created_before, instance_url):
       to_time(stats.median_latency))
 
 
-def print_count(reviewer, created_after, created_before, instance_url):
-  remote = rietveld.Rietveld(instance_url, None, None)
+def print_count(
+    reviewer, created_after, created_before, instance_url, auth_config):
+  remote = rietveld.Rietveld(instance_url, auth_config)
   print len(list(remote.search(
       reviewer=reviewer,
       created_after=created_after,
@@ -332,10 +335,12 @@ def main():
       '-i', '--instance_url', metavar='<host>',
       default='http://codereview.chromium.org',
       help='Host to use, default is %default')
+  auth.add_auth_options(parser)
   # Remove description formatting
   parser.format_description = (
       lambda _: parser.description)  # pylint: disable=E1101
   options, args = parser.parse_args()
+  auth_config = auth.extract_auth_config_from_options(options)
   if args:
     parser.error('Args unsupported')
   if options.reviewer is None:
@@ -363,13 +368,15 @@ def main():
         options.reviewer,
         options.begin,
         options.end,
-        options.instance_url)
+        options.instance_url,
+        auth_config)
   else:
     print_reviews(
         options.reviewer,
         options.begin,
         options.end,
-        options.instance_url)
+        options.instance_url,
+        auth_config)
   return 0
 
 

presubmit_support.py

@@ -39,6 +39,7 @@ import urllib2  # Exposed through the API.
 from warnings import warn
 
 # Local imports.
+import auth
 import fix_encoding
 import gclient_utils
 import owners
@@ -1637,7 +1638,6 @@ def main(argv=None):
                     "to skip multiple canned checks.")
   parser.add_option("--rietveld_url", help=optparse.SUPPRESS_HELP)
   parser.add_option("--rietveld_email", help=optparse.SUPPRESS_HELP)
-  parser.add_option("--rietveld_password", help=optparse.SUPPRESS_HELP)
   parser.add_option("--rietveld_fetch", action='store_true', default=False,
                     help=optparse.SUPPRESS_HELP)
   # These are for OAuth2 authentication for bots. See also apply_issue.py
@@ -1646,7 +1646,9 @@ def main(argv=None):
 
   parser.add_option("--trybot-json",
                     help="Output trybot information to the file specified.")
+  auth.add_auth_options(parser)
   options, args = parser.parse_args(argv)
+  auth_config = auth.extract_auth_config_from_options(options)
 
   if options.verbose >= 2:
     logging.basicConfig(level=logging.DEBUG)
@@ -1658,9 +1660,6 @@ def main(argv=None):
   if options.rietveld_email and options.rietveld_email_file:
     parser.error("Only one of --rietveld_email or --rietveld_email_file "
                  "can be passed to this program.")
-  if options.rietveld_private_key_file and options.rietveld_password:
-    parser.error("Only one of --rietveld_private_key_file or "
-                 "--rietveld_password can be passed to this program.")
 
   if options.rietveld_email_file:
     with open(options.rietveld_email_file, "rb") as f:
@@ -1682,8 +1681,8 @@ def main(argv=None):
     else:
       rietveld_obj = rietveld.CachingRietveld(
         options.rietveld_url,
-        options.rietveld_email,
-        options.rietveld_password)
+        auth_config,
+        options.rietveld_email)
     if options.rietveld_fetch:
       assert options.issue
       props = rietveld_obj.get_issue_properties(options.issue, False)

rietveld.py

@@ -37,27 +37,10 @@ upload.LOGGER.setLevel(logging.WARNING)  # pylint: disable=E1103
 
 class Rietveld(object):
   """Accesses rietveld."""
-  def __init__(self, url, email, password, extra_headers=None, maxtries=None):
+  def __init__(
+      self, url, auth_config, email=None, extra_headers=None, maxtries=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.
-    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
-        # setting the auth_function to None.
-        self.rpc_server = upload.HttpRpcServer(url, None)
-      else:
-        self.rpc_server = upload.GetRpcServer(url, email)
+    self.rpc_server = upload.GetRpcServer(self.url, auth_config, email)
 
     self._xsrf_token = None
     self._xsrf_token_time = None

tests/gcl_unittest.py

@@ -104,7 +104,7 @@ class GclUnittest(GclTestsBase):
         'OptionallyDoPresubmitChecks', 'REPOSITORY_ROOT',
         'RunShell', 'RunShellWithReturnCode', 'SVN',
         'TryChange', 'UnknownFiles', 'Warn',
-        'attrs', 'breakpad', 'defer_attributes', 'fix_encoding',
+        'attrs', 'auth', 'breakpad', 'defer_attributes', 'fix_encoding',
         'gclient_utils', 'git_cl', 'json', 'main', 'need_change',
         'need_change_and_args', 'no_args', 'optparse', 'os',
         'presubmit_support', 'random', 're', 'rietveld',

tests/git_cl_test.py

@@ -361,6 +361,7 @@ class TestGitCl(TestCase):
     return [
         'upload', '--assume_yes', '--server',
         'https://codereview.example.com',
+        '--no-oauth2', '--auth-host-port', '8090',
         '--message', description
     ] + args + [
         '--cc', 'joe@example.com',

tests/presubmit_unittest.py

@@ -171,7 +171,7 @@ class PresubmitUnittest(PresubmitTestsBase):
       'GitChange', 'InputApi', 'ListRelevantPresubmitFiles', 'main',
       'NonexistantCannedCheckFilter', 'OutputApi', 'ParseFiles',
       'PresubmitFailure', 'PresubmitExecuter', 'PresubmitOutput', 'ScanSubDirs',
-      'SvnAffectedFile', 'SvnChange', 'cPickle', 'cpplint', 'cStringIO',
+      'SvnAffectedFile', 'SvnChange', 'auth', 'cPickle', 'cpplint', 'cStringIO',
       'contextlib', 'canned_check_filter', 'fix_encoding', 'fnmatch',
       'gclient_utils', 'glob', 'inspect', 'json', 'load_files', 'logging',
       'marshal', 'normpath', 'optparse', 'os', 'owners', 'pickle',

tests/rietveld_test.py

@@ -47,7 +47,7 @@ class BaseFixture(unittest.TestCase):
     super(BaseFixture, self).setUp()
     # Access to a protected member XX of a client class
     # pylint: disable=W0212
-    self.rietveld = self.TESTED_CLASS('url', 'email', 'password')
+    self.rietveld = self.TESTED_CLASS('url', None, 'email')
     self.rietveld._send = self._rietveld_send
     self.requests = []
 
@@ -456,7 +456,7 @@ class DefaultTimeoutTest(auto_stub.TestCase):
 
   def setUp(self):
     super(DefaultTimeoutTest, self).setUp()
-    self.rietveld = self.TESTED_CLASS('url', 'email', 'password')
+    self.rietveld = self.TESTED_CLASS('url', None, 'email')
     self.mock(self.rietveld.rpc_server, 'Send', MockSend)
     self.sleep_time = 0
 

third_party/upload.py

@@ -72,6 +72,10 @@ try:
 except:
   keyring = None
 
+# auth.py is a part of depot_tools.
+# TODO(vadimsh): Merge upload.py into depot_tools
+import auth
+
 # The logging verbosity:
 #  0: Errors only.
 #  1: Status messages.
@@ -618,32 +622,11 @@ group.add_option("-s", "--server", action="store", dest="server",
 group.add_option("-e", "--email", action="store", dest="email",
                  metavar="EMAIL", default=None,
                  help="The username to use. Will prompt if omitted.")
-group.add_option("-H", "--host", action="store", dest="host",
-                 metavar="HOST", default=None,
-                 help="Overrides the Host header sent with all RPCs.")
-group.add_option("--no_cookies", action="store_false",
-                 dest="save_cookies", default=True,
-                 help="Do not save authentication cookies to local disk.")
-group.add_option("--oauth2", action="store_true",
-                 dest="use_oauth2", default=False,
-                 help="Use OAuth 2.0 instead of a password.")
-group.add_option("--oauth2_port", action="store", type="int",
-                 dest="oauth2_port", default=DEFAULT_OAUTH2_PORT,
-                 help=("Port to use to handle OAuth 2.0 redirect. Must be an "
-                       "integer in the range 1024-49151, defaults to "
-                       "'%default'."))
-group.add_option("--no_oauth2_webbrowser", action="store_false",
-                 dest="open_oauth2_local_webbrowser", default=True,
-                 help="Don't open a browser window to get an access token.")
-group.add_option("--account_type", action="store", dest="account_type",
-                 metavar="TYPE", default=AUTH_ACCOUNT_TYPE,
-                 choices=["GOOGLE", "HOSTED"],
-                 help=("Override the default account type "
-                       "(defaults to '%default', "
-                       "valid choices are 'GOOGLE' and 'HOSTED')."))
 group.add_option("-j", "--number-parallel-uploads",
                  dest="num_upload_threads", default=8,
                  help="Number of uploads to do in parallel.")
+# Authentication
+auth.add_auth_options(parser)
 # Issue
 group = parser.add_option_group("Issue options")
 group.add_option("-t", "--title", action="store", dest="title",
@@ -934,32 +917,28 @@ class OAuth2Creds(object):
                           open_local_webbrowser=self.open_local_webbrowser)
 
 
-def GetRpcServer(server, email=None, host_override=None, save_cookies=True,
-                 account_type=AUTH_ACCOUNT_TYPE, use_oauth2=False,
-                 oauth2_port=DEFAULT_OAUTH2_PORT,
-                 open_oauth2_local_webbrowser=True):
+def GetRpcServer(server, auth_config=None, email=None):
   """Returns an instance of an AbstractRpcServer.
 
   Args:
     server: String containing the review server URL.
-    email: String containing user's email address.
-    host_override: If not None, string containing an alternate hostname to use
-      in the host header.
-    save_cookies: Whether authentication cookies should be saved to disk.
-    account_type: Account type for authentication, either 'GOOGLE'
-      or 'HOSTED'. Defaults to AUTH_ACCOUNT_TYPE.
-    use_oauth2: Boolean indicating whether OAuth 2.0 should be used for
-      authentication.
-    oauth2_port: Integer, the port where the localhost server receiving the
-      redirect is serving. Defaults to DEFAULT_OAUTH2_PORT.
-    open_oauth2_local_webbrowser: Boolean, defaults to True. If True and using
-      OAuth, this opens a page in the user's browser to obtain a token.
+    auth_config: auth.AuthConfig tuple with OAuth2 configuration.
+    email: String containing user's email address [deprecated].
 
   Returns:
     A new HttpRpcServer, on which RPC calls can be made.
   """
+  # If email is given as an empty string or no auth config is passed, then
+  # assume we want to make requests that do not need authentication. Bypass
+  # authentication by setting the auth_function to None.
+  if email == '' or not auth_config:
+    return HttpRpcServer(server, None)
+
+  if auth_config.use_oauth2:
+    raise NotImplementedError('See https://crbug.com/356813')
+
   # If this is the dev_appserver, use fake authentication.
-  host = (host_override or server).lower()
+  host = server.lower()
   if re.match(r'(http://)?localhost([:/]|$)', host):
     if email is None:
       email = "test@example.com"
@@ -967,25 +946,19 @@ def GetRpcServer(server, email=None, host_override=None, save_cookies=True,
     server = HttpRpcServer(
         server,
         lambda: (email, "password"),
-        host_override=host_override,
         extra_headers={"Cookie":
                        'dev_appserver_login="%s:False"' % email},
-        save_cookies=save_cookies,
-        account_type=account_type)
+        save_cookies=auth_config.save_cookies,
+        account_type=AUTH_ACCOUNT_TYPE)
     # Don't try to talk to ClientLogin.
     server.authenticated = True
     return server
 
-  positional_args = [server]
-  if use_oauth2:
-    positional_args.append(
-        OAuth2Creds(server, oauth2_port, open_oauth2_local_webbrowser))
-  else:
-    positional_args.append(KeyringCreds(server, host, email).GetUserCredentials)
-  return HttpRpcServer(*positional_args,
-                       host_override=host_override,
-                       save_cookies=save_cookies,
-                       account_type=account_type)
+  return HttpRpcServer(
+      server,
+      KeyringCreds(server, host, email).GetUserCredentials,
+      save_cookies=auth_config.save_cookies,
+      account_type=AUTH_ACCOUNT_TYPE)
 
 
 def EncodeMultipartFormData(fields, files):
@@ -2598,16 +2571,9 @@ def RealMain(argv, data=None):
   files = vcs.GetBaseFiles(data)
   if verbosity >= 1:
     print "Upload server:", options.server, "(change with -s/--server)"
-  if options.use_oauth2:
-    options.save_cookies = False
-  rpc_server = GetRpcServer(options.server,
-                            options.email,
-                            options.host,
-                            options.save_cookies,
-                            options.account_type,
-                            options.use_oauth2,
-                            options.oauth2_port,
-                            options.open_oauth2_local_webbrowser)
+
+  auth_config = auth.extract_auth_config_from_options(options)
+  rpc_server = GetRpcServer(options.server, auth_config, options.email)
   form_fields = []
 
   repo_guid = vcs.GetGUID()