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
diff --git a/apply_issue.py b/apply_issue.py
index 2b7b334..41e133b 100755
--- a/apply_issue.py
+++ b/apply_issue.py
@@ -18,6 +18,7 @@
import breakpad # pylint: disable=W0611
import annotated_gclient
+import auth
import checkout
import fix_encoding
import gclient_utils
@@ -56,13 +57,10 @@
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 @@
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 @@
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 @@
# 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 @@
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 @@
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 @@
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)
diff --git a/auth.py b/auth.py
new file mode 100644
index 0000000..97520de
--- /dev/null
+++ b/auth.py
@@ -0,0 +1,99 @@
+# 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
diff --git a/commit_queue.py b/commit_queue.py
index 1bbf8a5..99eea34 100755
--- a/commit_queue.py
+++ b/commit_queue.py
@@ -17,6 +17,7 @@
import breakpad # pylint: disable=W0611
+import auth
import fix_encoding
import rietveld
@@ -36,9 +37,10 @@
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 @@
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)
diff --git a/gcl.py b/gcl.py
index cf65789..16f4f28 100755
--- a/gcl.py
+++ b/gcl.py
@@ -23,6 +23,7 @@
import breakpad # pylint: disable=W0611
+import auth
import fix_encoding
import gclient_utils
import git_cl
@@ -351,7 +352,10 @@
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 @@
'\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])
diff --git a/git_cherry_pick_upload.py b/git_cherry_pick_upload.py
index 0bcb9fd..3090364 100755
--- a/git_cherry_pick_upload.py
+++ b/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 @@
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 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
diff --git a/git_cl.py b/git_cl.py
index 65c7fc7..5484713 100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -34,6 +34,7 @@
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 @@
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 @@
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 @@
"""
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 @@
'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 @@
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 @@
# 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 @@
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 @@
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 @@
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 @@
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 @@
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 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 @@
"""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 @@
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 @@
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 @@
"""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 @@
'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 @@
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 @@
"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 @@
'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 @@
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 @@
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 @@
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 @@
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 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_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 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 @@
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 @@
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 @@
'--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:
diff --git a/my_activity.py b/my_activity.py
index 720d925..35ec4d8 100755
--- a/my_activity.py
+++ b/my_activity.py
@@ -13,6 +13,9 @@
- 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 urllib
import urllib2
+import auth
import gerrit_util
import rietveld
from third_party import upload
@@ -267,7 +271,8 @@
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 @@
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 = (
diff --git a/my_reviews.py b/my_reviews.py
index 6b36c56..a26aa02 100755
--- a/my_reviews.py
+++ b/my_reviews.py
@@ -14,6 +14,7 @@
import os
import sys
+import auth
import rietveld
@@ -214,9 +215,10 @@
', '.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 @@
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 @@
'-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 @@
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
diff --git a/presubmit_support.py b/presubmit_support.py
index a95b9ad..0abd421 100755
--- a/presubmit_support.py
+++ b/presubmit_support.py
@@ -39,6 +39,7 @@
from warnings import warn
# Local imports.
+import auth
import fix_encoding
import gclient_utils
import owners
@@ -1637,7 +1638,6 @@
"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 @@
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 @@
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 @@
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)
diff --git a/rietveld.py b/rietveld.py
index f3bcd71..0114cfe 100644
--- a/rietveld.py
+++ b/rietveld.py
@@ -37,27 +37,10 @@
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
diff --git a/tests/gcl_unittest.py b/tests/gcl_unittest.py
index b378744..0391101 100755
--- a/tests/gcl_unittest.py
+++ b/tests/gcl_unittest.py
@@ -104,7 +104,7 @@
'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',
diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py
index 71e8c60..b7d41f7 100755
--- a/tests/git_cl_test.py
+++ b/tests/git_cl_test.py
@@ -361,6 +361,7 @@
return [
'upload', '--assume_yes', '--server',
'https://codereview.example.com',
+ '--no-oauth2', '--auth-host-port', '8090',
'--message', description
] + args + [
'--cc', 'joe@example.com',
diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py
index 2196d8d..ebafb67 100755
--- a/tests/presubmit_unittest.py
+++ b/tests/presubmit_unittest.py
@@ -171,7 +171,7 @@
'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',
diff --git a/tests/rietveld_test.py b/tests/rietveld_test.py
index 20edb50..7b8c5ba 100755
--- a/tests/rietveld_test.py
+++ b/tests/rietveld_test.py
@@ -47,7 +47,7 @@
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 @@
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
diff --git a/third_party/upload.py b/third_party/upload.py
index a36aa9c..ca8c7b3 100755
--- a/third_party/upload.py
+++ b/third_party/upload.py
@@ -72,6 +72,10 @@
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("-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 @@
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 @@
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 @@
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()
