OAuth2 support in depot_tools

git_cl_oauth2.py

Index: git_cl_oauth2.py
diff --git a/git_cl_oauth2.py b/git_cl_oauth2.py
new file mode 100644
index 0000000000000000000000000000000000000000..cbfb9128e644bf8c6cb25571082045a86537b4e7
--- /dev/null
+++ b/git_cl_oauth2.py
@@ -0,0 +1,98 @@
+#!/usr/bin/env python
+# 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.
+
+"""OAuth2 related utilities and implementation got git cl commands."""

[nodir, 2015/03/05 03:58:56]

what "got" means here? Did you mean "for"?

[sheyang, 2015/03/06 00:29:10]

Yep... Fixed.

+
+import copy
+import logging
+import optparse
+import os
+
+import third_party.oauth2client.client as oa2client
+from third_party.oauth2client.file import Storage
+from third_party.oauth2client import tools
+from third_party.google_api_python_client import apiclient

[nodir, 2015/03/05 03:58:55]

sort imports

[sheyang, 2015/03/06 00:29:10]

Done.

+
+
+REDIRECT_URI = 'urn:ietf:wg:oauth:2.0:oob'
+CLIENT_SECRETS = {
+  'client_id': ('174799409470-8k3b89iov4racu9jrf7if3k4591voig3'
+                '.apps.googleusercontent.com'),
+  'client_secret': 'DddcCK1d6_ADwxqGDEGlsisy'

[nodir, 2015/03/05 03:58:56]

Make two constants instead of one dict

[sheyang, 2015/03/06 00:29:10]

Done.

+}
+
+
+def _fetch_storage(code_review_server):
+  storage_dir = os.path.expanduser('~/git_cl_credentials')

[nodir, 2015/03/05 03:58:56]

Does this work on windows? I'd use os.path.expanduser(os.path.join('~',
'.git_cl_credentials') instead

Also make the directory private: prefix with .

[sheyang, 2015/03/06 00:29:10]

Done.

+  if not os.path.isdir(storage_dir):
+    os.makedirs(storage_dir)
+  storage_path = os.path.join(storage_dir, '.%s' % code_review_server)

[nodir, 2015/03/05 03:58:55]

I'd make the parent dir invisible, and the files visible

[sheyang, 2015/03/06 00:29:10]

Done.

+  storage = Storage(storage_path)
+  return storage
+

[nodir, 2015/03/05 03:58:55]

nit: double blank lines between top-level definitions everywhere

[sheyang, 2015/03/06 00:29:10]

Done.

+def _fetch_cred_from_storage(storage):
+  print 'Fetching OAuth2 credential from local storage ...'

[nodir, 2015/03/05 03:58:55]

Use logging.info or logging.debug instead of print in this file. This one should
be probably debug

[nodir, 2015/03/05 03:58:56]

Please use plural form "credentials", "creds", everywhere in this file
(docstrings, vars, funcs)

[sheyang, 2015/03/06 00:29:10]

Done.

[sheyang, 2015/03/06 00:29:10]

Done.

+  credentials = storage.get()
+  if not credentials or credentials.invalid:
+    return None
+  if not credentials.access_token or credentials.access_token_expired:
+    return None
+  return credentials
+
+def add_oauth2_options(parser):
+  """Add OAuth2-related options."""
+  group = optparse.OptionGroup(parser, "OAuth2 options")
+  group.add_option(
+      '--auth_host_name',
+      default='localhost',
+      help='Host name to use when running a local web server '
+           'to handle redirects during OAuth authorization.'
+           'Default: localhost.'
+  )
+  group.add_option(
+      '--auth-host-port',

[nodir, 2015/03/05 03:58:56]

use either - or _ between words in options, be consistent. I think chrome-infra
standard is dashes

[sheyang, 2015/03/06 00:29:10]

use - now.

+      type=int,
+      action="append",

[nodir, 2015/03/05 03:58:56]

Use single quotes everywhere

[sheyang, 2015/03/06 00:29:10]

Copy-Paste error... Fixed.

+      default=[8080, 8090],
+      help='Port to use when running a local web server to handle '
+           'redirects during OAuth authorization. '
+           'Repeat this option to specify a list of values.'
+           'Default: [8080, 8090].'
+  )
+  group.add_option(
+      '--noauth_local_webserver',
+      action="store_true",
+      default=False,
+      help='Run a local web server to handle redirects '
+           'during OAuth authorization.'
+           'Default: False.'
+  )
+  group.add_option(
+      '--no_cache',
+      action="store_true",
+      default=False,
+      help='Get a new credential from web server instead of using '
+           'the crendential stored on a local storage file.'
+           'Default: False.'
+  )
+  parser.add_option_group(group)
+
+def get_oauth2_cred(options, code_review_server):
+  """Get OAuth2 credential."""

[nodir, 2015/03/05 03:58:56]

Please document args. Is code_review_server a hostname or URL with a schema? If
former, I'dcall it code_review_hostname

[sheyang, 2015/03/06 00:29:10]

CODE_REVIEW_SERVER is the same key name used in codereview.settings, so I'd
prefer to leave it as is.

Added docs.

+  storage = _fetch_storage(code_review_server)
+  cred = None
+  if not options.no_cache:
+    cred = _fetch_cred_from_storage(storage)
+  if cred is None:
+    print 'Fetching OAuth2 credential from web server...'
+    flow = oa2client.OAuth2WebServerFlow(
+        client_id=CLIENT_SECRETS['client_id'],
+        client_secret=CLIENT_SECRETS['client_secret'],
+        scope='email',

[nodir, 2015/03/05 03:58:56]

please make it a constant

[sheyang, 2015/03/06 00:29:10]

Done.

+        redirect_uri=REDIRECT_URI)
+    flags = copy.deepcopy(options)
+    flags.logging_level = 'WARNING'
+    cred = tools.run_flow(flow, storage, flags)
+  return cred