diff --git a/apply_issue.py b/apply_issue.py index 2b7b334f51..41e133baa0 100755 --- a/apply_issue.py +++ b/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) diff --git a/auth.py b/auth.py new file mode 100644 index 0000000000..97520deca3 --- /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 1bbf8a5c9e..99eea34ec0 100755 --- a/commit_queue.py +++ b/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) diff --git a/gcl.py b/gcl.py index cf657893c7..16f4f28273 100755 --- a/gcl.py +++ b/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]) diff --git a/git_cherry_pick_upload.py b/git_cherry_pick_upload.py index 0bcb9fd926..3090364c67 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 @@ 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 ') + parser.add_option( '--branch', '-b', help='The upstream branch to cherry pick to.', - metavar='', - required=True, - ) - parser.add_argument( - 'commit', - help='SHA to cherry pick.', - metavar='', - ) - args = parser.parse_args() - cherry_pick(args.branch, args.commit) + metavar='') + 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 ') + + cherry_pick(options.branch, args[0], auth_config) return 0 diff --git a/git_cl.py b/git_cl.py index 65c7fc79f9..5484713c57 100755 --- a/git_cl.py +++ b/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 '") 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: diff --git a/my_activity.py b/my_activity.py index 720d9255cd..35ec4d87f3 100755 --- a/my_activity.py +++ b/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 = ( diff --git a/my_reviews.py b/my_reviews.py index 6b36c5682c..a26aa02a2e 100755 --- a/my_reviews.py +++ b/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='', 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 diff --git a/presubmit_support.py b/presubmit_support.py index a95b9adf6e..0abd421910 100755 --- a/presubmit_support.py +++ b/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) diff --git a/rietveld.py b/rietveld.py index f3bcd718d3..0114cfe0cc 100644 --- a/rietveld.py +++ b/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 diff --git a/tests/gcl_unittest.py b/tests/gcl_unittest.py index b3787447be..0391101252 100755 --- a/tests/gcl_unittest.py +++ b/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', diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 71e8c60ccf..b7d41f7e56 100755 --- a/tests/git_cl_test.py +++ b/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', diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 2196d8d638..ebafb67b1b 100755 --- a/tests/presubmit_unittest.py +++ b/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', diff --git a/tests/rietveld_test.py b/tests/rietveld_test.py index 20edb5056f..7b8c5baeca 100755 --- a/tests/rietveld_test.py +++ b/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 diff --git a/third_party/upload.py b/third_party/upload.py index a36aa9c888..ca8c7b3db8 100755 --- a/third_party/upload.py +++ b/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()