depot_tools: Clean up auth.py

Bug: 1001756
Change-Id: I495991c30f7f00de17e7c098e2d88ef7783aff3c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1865265
Auto-Submit: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Vadim Shtayura <vadimsh@chromium.org>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

gerrit_util.py

@@ -351,17 +351,11 @@ class LuciContextAuthenticator(Authenticator):
     return auth.has_luci_context_local_auth()
 
   def __init__(self):
-    self._access_token = None
-    self._ensure_fresh()
-
-  def _ensure_fresh(self):
-    if not self._access_token or self._access_token.needs_refresh():
-      self._access_token = auth.get_luci_context_access_token(
-          scopes=' '.join([auth.OAUTH_SCOPE_EMAIL, auth.OAUTH_SCOPE_GERRIT]))
+    self._authenticator = auth.Authenticator(
+        ' '.join([auth.OAUTH_SCOPE_EMAIL, auth.OAUTH_SCOPE_GERRIT]))
 
   def get_auth_header(self, _host):
-    self._ensure_fresh()
-    return 'Bearer %s' % self._access_token.token
+    return 'Bearer %s' % self._authenticator.get_access_token().token
 
 
 def CreateHttpConn(host, path, reqtype='GET', headers=None, body=None):

git_cl.py

@@ -443,11 +443,10 @@ def _parse_bucket(raw_bucket):
   return project, bucket
 
 
-def _trigger_try_jobs(auth_config, changelist, buckets, options, patchset):
+def _trigger_try_jobs(changelist, buckets, options, patchset):
   """Sends a request to Buildbucket to trigger tryjobs for a changelist.
 
   Args:
-    auth_config: AuthConfig for Buildbucket.
     changelist: Changelist that the tryjobs are associated with.
     buckets: A nested dict mapping bucket names to builders to tests.
     options: Command-line options.
@@ -466,7 +465,7 @@ def _trigger_try_jobs(auth_config, changelist, buckets, options, patchset):
   if not requests:
     return
 
-  http = auth.get_authenticator(auth_config).authorize(httplib2.Http())
+  http = auth.Authenticator().authorize(httplib2.Http())
   http.force_exception_to_status_code = True
 
   batch_request = {'requests': requests}
@@ -527,8 +526,7 @@ def _make_try_job_schedule_requests(changelist, buckets, options, patchset):
   return requests
 
 
-def fetch_try_jobs(auth_config, changelist, buildbucket_host,
-                   patchset=None):
+def fetch_try_jobs(changelist, buildbucket_host, patchset=None):
   """Fetches tryjobs from buildbucket.
 
   Returns list of buildbucket.v2.Build with the try jobs for the changelist.
@@ -541,7 +539,7 @@ def fetch_try_jobs(auth_config, changelist, buildbucket_host,
       'fields': ','.join('builds.*.' + field for field in fields),
   }
 
-  authenticator = auth.get_authenticator(auth_config)
+  authenticator = auth.Authenticator()
   if authenticator.has_cached_credentials():
     http = authenticator.authorize(httplib2.Http())
   else:
@@ -554,13 +552,11 @@ def fetch_try_jobs(auth_config, changelist, buildbucket_host,
   response = _call_buildbucket(http, buildbucket_host, 'SearchBuilds', request)
   return response.get('builds', [])
 
-def _fetch_latest_builds(
-    auth_config, changelist, buildbucket_host, latest_patchset=None):
+def _fetch_latest_builds(changelist, buildbucket_host, latest_patchset=None):
   """Fetches builds from the latest patchset that has builds (within
   the last few patchsets).
 
   Args:
-    auth_config (auth.AuthConfig): Auth info for Buildbucket
     changelist (Changelist): The CL to fetch builds for
     buildbucket_host (str): Buildbucket host, e.g. "cr-buildbucket.appspot.com"
     lastest_patchset(int|NoneType): the patchset to start fetching builds from.
@@ -581,8 +577,7 @@ def _fetch_latest_builds(
 
   min_ps = max(1, ps - 5)
   while ps >= min_ps:
-    builds = fetch_try_jobs(
-        auth_config, changelist, buildbucket_host, patchset=ps)
+    builds = fetch_try_jobs(changelist, buildbucket_host, patchset=ps)
     if len(builds):
       return builds, ps
     ps -= 1
@@ -4439,7 +4434,6 @@ def CMDupload(parser, args):
                          'fixed (pre-populates "Fixed:" tag). Same format as '
                          '-b option / "Bug:" tag. If fixing several issues, '
                          'separate with commas.')
-  auth.add_auth_options(parser)
 
   orig_args = args
   _add_codereview_select_options(parser)
@@ -4487,15 +4481,13 @@ def CMDupload(parser, args):
     if ret != 0:
       print('Upload failed, so --retry-failed has no effect.')
       return ret
-    auth_config = auth.extract_auth_config_from_options(options)
     builds, _ = _fetch_latest_builds(
-        auth_config, cl, options.buildbucket_host,
-        latest_patchset=patchset)
+        cl, options.buildbucket_host, latest_patchset=patchset)
     buckets = _filter_failed_for_retry(builds)
     if len(buckets) == 0:
       print('No failed tryjobs, so --retry-failed has no effect.')
       return ret
-    _trigger_try_jobs(auth_config, cl, buckets, options, patchset + 1)
+    _trigger_try_jobs(cl, buckets, options, patchset + 1)
 
   return ret
 
@@ -4745,10 +4737,8 @@ def CMDtry(parser, args):
       '-R', '--retry-failed', action='store_true', default=False,
       help='Retry failed jobs from the latest set of tryjobs. '
            'Not allowed with --bucket and --bot options.')
-  auth.add_auth_options(parser)
   _add_codereview_issue_select_options(parser)
   options, args = parser.parse_args(args)
-  auth_config = auth.extract_auth_config_from_options(options)
 
   # Make sure that all properties are prop=value pairs.
   bad_params = [x for x in options.properties if '=' not in x]
@@ -4775,8 +4765,7 @@ def CMDtry(parser, args):
             '-B, -b, --bucket, or --bot.', file=sys.stderr)
       return 1
     print('Searching for failed tryjobs...')
-    builds, patchset = _fetch_latest_builds(
-        auth_config, cl, options.buildbucket_host)
+    builds, patchset = _fetch_latest_builds(cl, options.buildbucket_host)
     if options.verbose:
       print('Got %d builds in patchset #%d' % (len(builds), patchset))
     buckets = _filter_failed_for_retry(builds)
@@ -4812,7 +4801,7 @@ def CMDtry(parser, args):
 
   patchset = cl.GetMostRecentPatchset()
   try:
-    _trigger_try_jobs(auth_config, cl, buckets, options, patchset)
+    _trigger_try_jobs(cl, buckets, options, patchset)
   except BuildbucketResponseException as ex:
     print('ERROR: %s' % ex)
     return 1
@@ -4837,13 +4826,11 @@ def CMDtry_results(parser, args):
       '--json', help=('Path of JSON output file to write tryjob results to,'
                       'or "-" for stdout.'))
   parser.add_option_group(group)
-  auth.add_auth_options(parser)
   _add_codereview_issue_select_options(parser)
   options, args = parser.parse_args(args)
   if args:
     parser.error('Unrecognized args: %s' % ' '.join(args))
 
-  auth_config = auth.extract_auth_config_from_options(options)
   cl = Changelist(issue=options.issue)
   if not cl.GetIssue():
     parser.error('Need to upload first.')
@@ -4858,7 +4845,7 @@ def CMDtry_results(parser, args):
                    cl.GetIssue())
 
   try:
-    jobs = fetch_try_jobs(auth_config, cl, options.buildbucket_host, patchset)
+    jobs = fetch_try_jobs(cl, options.buildbucket_host, patchset)
   except BuildbucketResponseException as ex:
     print('Buildbucket error: %s' % ex)
     return 1
@@ -5470,7 +5457,7 @@ def main(argv):
   dispatcher = subcommand.CommandDispatcher(__name__)
   try:
     return dispatcher.execute(OptionParser(), argv)
-  except auth.AuthenticationError as e:
+  except auth.LoginRequiredError as e:
     DieWithError(str(e))
   except urllib2.HTTPError as e:
     if e.code != 500:

my_activity.py

@@ -292,12 +292,10 @@ class MyActivity(object):
     return ret
 
   def monorail_get_auth_http(self):
-    auth_config = auth.extract_auth_config_from_options(self.options)
-    authenticator = auth.get_authenticator(auth_config)
     # Manually use a long timeout (10m); for some users who have a
     # long history on the issue tracker, whatever the default timeout
     # is is reached.
-    return authenticator.authorize(httplib2.Http(timeout=600))
+    return auth.Authenticator().authorize(httplib2.Http(timeout=600))
 
   def filter_modified_monorail_issue(self, issue):
     """Precisely checks if an issue has been modified in the time range.
@@ -809,7 +807,6 @@ def main():
       '-j', '--json', action='store_true',
       help='Output json data (overrides other format options)')
   parser.add_option_group(output_format_group)
-  auth.add_auth_options(parser)
 
   parser.add_option(
       '-v', '--verbose',
@@ -925,8 +922,8 @@ def main():
       my_activity.get_issues()
     if not options.no_referenced_issues:
       my_activity.get_referenced_issues()
-  except auth.AuthenticationError as e:
-    logging.error('auth.AuthenticationError: %s', e)
+  except auth.LoginRequiredError as e:
+    logging.error('auth.LoginRequiredError: %s', e)
 
   my_activity.show_progress('\n')
 

presubmit_canned_checks.py

@@ -1417,9 +1417,8 @@ def CheckChangedLUCIConfigs(input_api, output_api):
 
   # authentication
   try:
-    authenticator = auth.get_authenticator(auth.make_auth_config())
-    acc_tkn = authenticator.get_access_token()
-  except auth.AuthenticationError as e:
+    acc_tkn = auth.Authenticator().get_access_token()
+  except auth.LoginRequiredError as e:
     return [output_api.PresubmitError(
         'Error in authenticating user.', long_text=str(e))]
 

tests/auth_test.py

@@ -34,42 +34,57 @@ class AuthenticatorTest(unittest.TestCase):
   def testHasCachedCredentials_NotLoggedIn(self):
     subprocess2.check_call_out.side_effect = [
         subprocess2.CalledProcessError(1, ['cmd'], 'cwd', 'stdout', 'stderr')]
-    authenticator = auth.get_authenticator(auth.make_auth_config())
-    self.assertFalse(authenticator.has_cached_credentials())
+    self.assertFalse(auth.Authenticator().has_cached_credentials())
 
   def testHasCachedCredentials_LoggedIn(self):
     subprocess2.check_call_out.return_value = (
         json.dumps({'token': 'token', 'expiry': 12345678}), '')
-    authenticator = auth.get_authenticator(auth.make_auth_config())
-    self.assertTrue(authenticator.has_cached_credentials())
+    self.assertTrue(auth.Authenticator().has_cached_credentials())
 
   def testGetAccessToken_NotLoggedIn(self):
     subprocess2.check_call_out.side_effect = [
         subprocess2.CalledProcessError(1, ['cmd'], 'cwd', 'stdout', 'stderr')]
-    authenticator = auth.get_authenticator(auth.make_auth_config())
-    self.assertRaises(auth.LoginRequiredError, authenticator.get_access_token)
+    self.assertRaises(
+        auth.LoginRequiredError, auth.Authenticator().get_access_token)
 
   def testGetAccessToken_CachedToken(self):
-    authenticator = auth.get_authenticator(auth.make_auth_config())
+    authenticator = auth.Authenticator()
     authenticator._access_token = auth.AccessToken('token', None)
     self.assertEqual(
         auth.AccessToken('token', None), authenticator.get_access_token())
+    self.assertEqual(0, len(subprocess2.check_call_out.mock_calls))
 
   def testGetAccesstoken_LoggedIn(self):
     expiry = calendar.timegm(VALID_EXPIRY.timetuple())
     subprocess2.check_call_out.return_value = (
         json.dumps({'token': 'token', 'expiry': expiry}), '')
-    authenticator = auth.get_authenticator(auth.make_auth_config())
     self.assertEqual(
         auth.AccessToken('token', VALID_EXPIRY),
-        authenticator.get_access_token())
+        auth.Authenticator().get_access_token())
+    subprocess2.check_call_out.assert_called_with(
+        ['luci-auth',
+         'token',
+         '-scopes', auth.OAUTH_SCOPE_EMAIL,
+         '-json-output', '-'],
+        stdout=subprocess2.PIPE, stderr=subprocess2.PIPE)
+
+  def testGetAccessToken_DifferentScope(self):
+    expiry = calendar.timegm(VALID_EXPIRY.timetuple())
+    subprocess2.check_call_out.return_value = (
+        json.dumps({'token': 'token', 'expiry': expiry}), '')
+    self.assertEqual(
+        auth.AccessToken('token', VALID_EXPIRY),
+        auth.Authenticator('custom scopes').get_access_token())
+    subprocess2.check_call_out.assert_called_with(
+        ['luci-auth', 'token', '-scopes', 'custom scopes', '-json-output', '-'],
+        stdout=subprocess2.PIPE, stderr=subprocess2.PIPE)
 
   def testAuthorize(self):
     http = mock.Mock()
     http_request = http.request
     http_request.__name__ = '__name__'
 
-    authenticator = auth.get_authenticator(auth.make_auth_config())
+    authenticator = auth.Authenticator()
     authenticator._access_token = auth.AccessToken('token', None)
 
     authorized = authenticator.authorize(http)

tests/git_cl_test.py

@@ -637,7 +637,7 @@ class TestGitCl(TestCase):
               self._mocked_call('write_json', path, contents))
     self.mock(git_cl.presubmit_support, 'DoPresubmitChecks', PresubmitMock)
     self.mock(git_cl.watchlists, 'Watchlists', WatchlistsMock)
-    self.mock(git_cl.auth, 'get_authenticator', AuthenticatorMock)
+    self.mock(git_cl.auth, 'Authenticator', AuthenticatorMock)
     self.mock(git_cl.gerrit_util, 'GetChangeDetail',
               lambda *args, **kwargs: self._mocked_call(
                   'GetChangeDetail', *args, **kwargs))
@@ -3062,7 +3062,7 @@ class CMDTestCaseBase(unittest.TestCase):
                return_value='https://chromium-review.googlesource.com').start()
     mock.patch('git_cl.Changelist.GetMostRecentPatchset',
                return_value=7).start()
-    mock.patch('git_cl.auth.get_authenticator',
+    mock.patch('git_cl.auth.Authenticator',
                return_value=AuthenticatorMock()).start()
     mock.patch('git_cl.Changelist._GetChangeDetail',
                return_value=self._CHANGE_DETAIL).start()
@@ -3382,14 +3382,14 @@ class CMDUploadTestCase(CMDTestCaseBase):
 
     self.assertEqual(0, git_cl.main(['upload', '--retry-failed']))
     self.assertEqual([
-        mock.call(mock.ANY, mock.ANY, 'cr-buildbucket.appspot.com', patchset=7),
-        mock.call(mock.ANY, mock.ANY, 'cr-buildbucket.appspot.com', patchset=6),
+        mock.call(mock.ANY, 'cr-buildbucket.appspot.com', patchset=7),
+        mock.call(mock.ANY, 'cr-buildbucket.appspot.com', patchset=6),
     ], git_cl.fetch_try_jobs.mock_calls)
     expected_buckets = {
         'chromium/try': {'bot_failure': [], 'bot_infra_failure': []},
     }
     git_cl._trigger_try_jobs.assert_called_once_with(
-        mock.ANY, mock.ANY, expected_buckets, mock.ANY, 8)
+        mock.ANY, expected_buckets, mock.ANY, 8)
 
 
 class CMDFormatTestCase(TestCase):

tests/presubmit_unittest.py

@@ -1642,7 +1642,7 @@ class CannedChecksUnittest(PresubmitTestsBase):
                      presubmit.OutputApi.PresubmitPromptWarning)
 
   @mock.patch('git_cl.Changelist')
-  @mock.patch('auth.get_authenticator')
+  @mock.patch('auth.Authenticator')
   def testCannedCheckChangedLUCIConfigs(self, mockGetAuth, mockChangelist):
     affected_file1 = mock.MagicMock(presubmit.GitAffectedFile)
     affected_file1.LocalPath.return_value = 'foo.cfg'