Gerrit git cl upload: add check for missing credentials.

Checks creds before uploading and running presubmit, generalizing
the case of Rietveld. If they are missing, suggests a URL to
generate them.

R=andybons@chromium.org,phajdan.jr@chromium.org
BUG=583153

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

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

gerrit_util.py

@@ -83,28 +83,47 @@ class Authenticator(object):
     """
     if GceAuthenticator.is_gce():
       return GceAuthenticator()
-    return NetrcAuthenticator()
+    return CookiesAuthenticator()
 
 
-class NetrcAuthenticator(Authenticator):
-  """Authenticator implementation that uses ".netrc" for token.
+class CookiesAuthenticator(Authenticator):
+  """Authenticator implementation that uses ".netrc" or ".gitcookies" for token.
+
+  Expected case for developer workstations.
   """
 
   def __init__(self):
     self.netrc = self._get_netrc()
     self.gitcookies = self._get_gitcookies()
 
-  @staticmethod
-  def _get_netrc():
+  @classmethod
+  def get_new_password_message(cls, host):
+    assert not host.startswith('http')
+    # Assume *.googlesource.com pattern.
+    parts = host.split('.')
+    if not parts[0].endswith('-review'):
+      parts[0] += '-review'
+    url = 'https://%s/new-password' % ('.'.join(parts))
+    return 'You can (re)generate your credentails by visiting %s' % url
+
+  @classmethod
+  def get_netrc_path(cls):
     path = '_netrc' if sys.platform.startswith('win') else '.netrc'
-    path = os.path.expanduser(os.path.join('~', path))
+    return os.path.expanduser(os.path.join('~', path))
+
+  @classmethod
+  def _get_netrc(cls):
+    path = cls.get_netrc_path()
+    if not os.path.exists(path):
+      return netrc.netrc(os.devnull)
+
     try:
       return netrc.netrc(path)
     except IOError:
       print >> sys.stderr, 'WARNING: Could not read netrc file %s' % path
       return netrc.netrc(os.devnull)
-    except netrc.NetrcParseError as e:
-      st = os.stat(e.path)
+    except netrc.NetrcParseError:
+      st = os.stat(path)
       if st.st_mode & (stat.S_IRWXG | stat.S_IRWXO):
         print >> sys.stderr, (
             'WARNING: netrc file %s cannot be used because its file '
@@ -116,10 +135,17 @@ class NetrcAuthenticator(Authenticator):
         raise
       return netrc.netrc(os.devnull)
 
-  @staticmethod
-  def _get_gitcookies():
+  @classmethod
+  def get_gitcookies_path(cls):
+    return os.path.join(os.environ['HOME'], '.gitcookies')
+
+  @classmethod
+  def _get_gitcookies(cls):
     gitcookies = {}
-    path = os.path.join(os.environ['HOME'], '.gitcookies')
+    path = cls.get_gitcookies_path()
+    if not os.path.exists(path):
+      return gitcookies
+
     try:
       f = open(path, 'rb')
     except IOError:
@@ -153,6 +179,10 @@ class NetrcAuthenticator(Authenticator):
       return 'Basic %s' % (base64.b64encode('%s:%s' % (auth[0], auth[2])))
     return None
 
+# Backwards compatibility just in case somebody imports this outside of
+# depot_tools.
+NetrcAuthenticator = CookiesAuthenticator
+
 
 class GceAuthenticator(Authenticator):
   """Authenticator implementation that uses GCE metadata service for token.

git_cl.py

@@ -1329,7 +1329,7 @@ class Changelist(object):
     # Make sure authenticated to codereview before running potentially expensive
     # hooks.  It is a fast, best efforts check. Codereview still can reject the
     # authentication during the actual upload.
-    self._codereview_impl.EnsureAuthenticated()
+    self._codereview_impl.EnsureAuthenticated(force=options.force)
 
     # Apply watchlists on upload.
     change = self.GetChange(base_branch, None)
@@ -1507,8 +1507,12 @@ class _ChangelistCodereviewBase(object):
     failed."""
     raise NotImplementedError()
 
-  def EnsureAuthenticated(self):
-    """Best effort check that user is authenticated with codereview server."""
+  def EnsureAuthenticated(self, force):
+    """Best effort check that user is authenticated with codereview server.
+
+    Arguments:
+      force: whether to skip confirmation questions.
+    """
     raise NotImplementedError()
 
   def CMDUploadChange(self, options, args, change):
@@ -1540,7 +1544,7 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase):
         self._rietveld_server = settings.GetDefaultServerUrl()
     return self._rietveld_server
 
-  def EnsureAuthenticated(self):
+  def EnsureAuthenticated(self, force):
     """Best effort check that user is authenticated with Rietveld server."""
     if self._auth_config.use_oauth2:
       authenticator = auth.get_authenticator_for_host(
@@ -1785,7 +1789,6 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase):
     """Upload the patch to Rietveld."""
     upload_args = ['--assume_yes']  # Don't ask about untracked files.
     upload_args.extend(['--server', self.GetCodereviewServer()])
-    # TODO(tandrii): refactor this ugliness into _RietveldChangelistImpl.
     upload_args.extend(auth.auth_config_to_command_options(self._auth_config))
     if options.emulate_svn_auto_props:
       upload_args.append('--emulate_svn_auto_props')
@@ -1939,14 +1942,19 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
     # auth_config is Rietveld thing, kept here to preserve interface only.
     super(_GerritChangelistImpl, self).__init__(changelist)
     self._change_id = None
+    # Lazily cached values.
     self._gerrit_server = None  # e.g. https://chromium-review.googlesource.com
-    self._gerrit_host = None   # e.g. chromium-review.googlesource.com
+    self._gerrit_host = None    # e.g. chromium-review.googlesource.com
 
   def _GetGerritHost(self):
     # Lazy load of configs.
     self.GetCodereviewServer()
     return self._gerrit_host
 
+  def _GetGitHost(self):
+    """Returns git host to be used when uploading change to Gerrit."""
+    return urlparse.urlparse(self.GetRemoteUrl()).netloc
+
   def GetCodereviewServer(self):
     if not self._gerrit_server:
       # If we're on a branch then get the server potentially associated
@@ -1961,7 +1969,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
       if not self._gerrit_server:
         # We assume repo to be hosted on Gerrit, and hence Gerrit server
         # has "-review" suffix for lowest level subdomain.
-        parts = urlparse.urlparse(self.GetRemoteUrl()).netloc.split('.')
+        parts = self._GetGitHost().split('.')
         parts[0] = parts[0] + '-review'
         self._gerrit_host = '.'.join(parts)
         self._gerrit_server = 'https://%s' % self._gerrit_host
@@ -1971,9 +1979,47 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
   def IssueSettingSuffix(cls):
     return 'gerritissue'
 
-  def EnsureAuthenticated(self):
+  def EnsureAuthenticated(self, force):
     """Best effort check that user is authenticated with Gerrit server."""
-    #TODO(tandrii): implement per bug http://crbug.com/583153.
+    # Lazy-loader to identify Gerrit and Git hosts.
+    if gerrit_util.GceAuthenticator.is_gce():
+      return
+    self.GetCodereviewServer()
+    git_host = self._GetGitHost()
+    assert self._gerrit_server and self._gerrit_host
+    cookie_auth = gerrit_util.CookiesAuthenticator()
+
+    gerrit_auth = cookie_auth.get_auth_header(self._gerrit_host)
+    git_auth = cookie_auth.get_auth_header(git_host)
+    if gerrit_auth and git_auth:
+      if gerrit_auth == git_auth:
+        return
+      print((
+          'WARNING: you have different credentials for Gerrit and git hosts.\n'
+          '         Check your %s or %s file for credentials of hosts:\n'
+          '           %s\n'
+          '           %s\n'
+          '         %s') %
+          (cookie_auth.get_gitcookies_path(), cookie_auth.get_netrc_path(),
+           git_host, self._gerrit_host,
+           cookie_auth.get_new_password_message(git_host)))
+      if not force:
+        ask_for_data('If you know what you are doing, press Enter to continue, '
+                     'Ctrl+C to abort.')
+      return
+    else:
+      missing = (
+          [] if gerrit_auth else [self._gerrit_host] +
+          [] if git_auth else [git_host])
+      DieWithError('Credentials for the following hosts are required:\n'
+                   '  %s\n'
+                   'These are read from %s (or legacy %s)\n'
+                   '%s' % (
+                     '\n  '.join(missing),
+                     cookie_auth.get_gitcookies_path(),
+                     cookie_auth.get_netrc_path(),
+                     cookie_auth.get_new_password_message(git_host)))
+
 
   def PatchsetSetting(self):
     """Return the git setting that stores this change's most recent patchset."""

tests/git_cl_test.py

@@ -74,6 +74,34 @@ class AuthenticatorMock(object):
     return True
 
 
+def CookiesAuthenticatorMockFactory(hosts_with_creds=None, same_cookie=False):
+  """Use to mock Gerrit/Git credentials from ~/.netrc or ~/.gitcookies.
+
+  Usage:
+    >>> self.mock(git_cl.gerrit_util, "CookiesAuthenticator",
+                  CookiesAuthenticatorMockFactory({'host1': 'cookie1'}))
+
+  OR
+    >>> self.mock(git_cl.gerrit_util, "CookiesAuthenticator",
+                  CookiesAuthenticatorMockFactory(cookie='cookie'))
+  """
+  class CookiesAuthenticatorMock(git_cl.gerrit_util.CookiesAuthenticator):
+    def __init__(self):  # pylint: disable=W0231
+      # Intentionally not calling super() because it reads actual cookie files.
+      pass
+    @classmethod
+    def get_gitcookies_path(cls):
+      return '~/.gitcookies'
+    @classmethod
+    def get_netrc_path(cls):
+      return '~/.netrc'
+    def get_auth_header(self, host):
+      if same_cookie:
+        return same_cookie
+      return (hosts_with_creds or {}).get(host)
+  return CookiesAuthenticatorMock
+
+
 class TestGitClBasic(unittest.TestCase):
   def _test_ParseIssueUrl(self, func, url, issue, patchset, hostname, fail):
     parsed = urlparse.urlparse(url)
@@ -191,6 +219,8 @@ class TestGitCl(TestCase):
     self.mock(git_cl.upload, 'RealMain', self.fail)
     self.mock(git_cl.watchlists, 'Watchlists', WatchlistsMock)
     self.mock(git_cl.auth, 'get_authenticator_for_host', AuthenticatorMock)
+    self.mock(git_cl.gerrit_util.GceAuthenticator, 'is_gce',
+              classmethod(lambda _: False))
     # It's important to reset settings to not have inter-tests interference.
     git_cl.settings = None
 
@@ -346,11 +376,10 @@ class TestGitCl(TestCase):
     ]
 
   @staticmethod
-  def _git_sanity_checks(diff_base, working_branch):
+  def _git_sanity_checks(diff_base, working_branch, get_remote_branch=True):
     fake_ancestor = 'fake_ancestor'
     fake_cl = 'fake_cl_for_patch'
     return [
-      # Calls to verify branch point is ancestor
       ((['git',
          'rev-parse', '--verify', diff_base],), fake_ancestor),
       ((['git',
@@ -360,15 +389,17 @@ class TestGitCl(TestCase):
       # Mock a config miss (error code 1)
       ((['git',
          'config', 'gitcl.remotebranch'],), (('', None), 1)),
+    ] + ([
       # Call to GetRemoteBranch()
       ((['git',
          'config', 'branch.%s.merge' % working_branch],),
        'refs/heads/master'),
       ((['git',
          'config', 'branch.%s.remote' % working_branch],), 'origin'),
+    ] if get_remote_branch else []) + [
       ((['git', 'rev-list', '^' + fake_ancestor,
          'refs/remotes/origin/master'],), ''),
-       ]
+    ]
 
   @classmethod
   def _dcommit_calls_1(cls):
@@ -644,6 +675,23 @@ class TestGitCl(TestCase):
     git_cl.main(['dcommit', '--bypass-hooks'])
 
 
+  @classmethod
+  def _gerrit_ensure_auth_calls(cls, issue=None):
+    calls = []
+    if issue:
+      calls.extend([
+          ((['git', 'config', 'branch.master.gerritserver'],), ''),
+      ])
+    calls.extend([
+        ((['git', 'config', 'branch.master.merge'],), 'refs/heads/master'),
+        ((['git', 'config', 'branch.master.remote'],), 'origin'),
+        ((['git', 'config', 'remote.origin.url'],),
+         'https://chromium.googlesource.com/my/repo'),
+        ((['git', 'config', 'remote.origin.url'],),
+         'https://chromium.googlesource.com/my/repo'),
+    ])
+    return calls
+
   @classmethod
   def _gerrit_base_calls(cls, issue=None):
     return [
@@ -658,13 +706,18 @@ class TestGitCl(TestCase):
         ((['git', 'config', 'branch.master.rietveldissue'],), ''),
         ((['git', 'config', 'branch.master.gerritissue'],),
           '' if issue is None else str(issue)),
-        ((['git', 'config', 'branch.master.merge'],), 'master'),
+        ((['git', 'config', 'branch.master.merge'],), 'refs/heads/master'),
         ((['git', 'config', 'branch.master.remote'],), 'origin'),
-        ((['get_or_create_merge_base', 'master', 'master'],),
+        ((['get_or_create_merge_base', 'master',
+           'refs/remotes/origin/master'],),
          'fake_ancestor_sha'),
-      ] + cls._git_sanity_checks('fake_ancestor_sha', 'master') + [
+        # Calls to verify branch point is ancestor
+      ] + (cls._gerrit_ensure_auth_calls(issue=issue) +
+           cls._git_sanity_checks('fake_ancestor_sha', 'master',
+                                  get_remote_branch=False)) + [
         ((['git', 'rev-parse', '--show-cdup'],), ''),
         ((['git', 'rev-parse', 'HEAD'],), '12345'),
+
         ((['git',
            'diff', '--name-status', '--no-renames', '-r',
            'fake_ancestor_sha...', '.'],),
@@ -730,7 +783,8 @@ class TestGitCl(TestCase):
            'refs/heads/master'),
           ((['git', 'config', 'branch.master.remote'],),
            'origin'),
-          ((['get_or_create_merge_base', 'master', 'master'],),
+          ((['get_or_create_merge_base', 'master',
+             'refs/remotes/origin/master'],),
            'origin/master'),
           ((['git', 'rev-parse', 'HEAD:'],),
            '0123456789abcdef'),
@@ -774,14 +828,11 @@ class TestGitCl(TestCase):
     if squash:
       calls += [
           ((['git', 'config', 'branch.master.gerritissue', '123456'],), ''),
-          ((['git', 'config', 'branch.master.gerritserver'],), ''),
-          ((['git', 'config', 'remote.origin.url'],),
-            'https://chromium.googlesource.com/my/repo.git'),
           ((['git', 'config', 'branch.master.gerritserver',
           'https://chromium-review.googlesource.com'],), ''),
           ((['git', 'config', 'branch.master.gerritsquashhash',
              'abcdef0123456789'],), ''),
-          ]
+      ]
     calls += cls._git_post_upload_calls()
     return calls
 
@@ -795,6 +846,8 @@ class TestGitCl(TestCase):
       post_amend_description=None,
       issue=None):
     """Generic gerrit upload test framework."""
+    self.mock(git_cl.gerrit_util, "CookiesAuthenticator",
+              CookiesAuthenticatorMockFactory(same_cookie='same_cred'))
     self.calls = self._gerrit_base_calls(issue=issue)
     self.calls += self._gerrit_upload_calls(
         description, reviewers, squash,
@@ -1193,6 +1246,50 @@ class TestGitCl(TestCase):
     ]
     self.assertEqual(1, git_cl.main(['checkout', '99999']))
 
+  def _test_gerrit_ensure_authenticated_common(self, auth):
+    self.mock(git_cl.gerrit_util, 'CookiesAuthenticator',
+              CookiesAuthenticatorMockFactory(hosts_with_creds=auth))
+    self.mock(git_cl, 'DieWithError',
+              lambda msg: self._mocked_call(['DieWithError', msg]))
+    self.mock(git_cl, 'ask_for_data',
+              lambda msg: self._mocked_call(['ask_for_data', msg]))
+    self.calls = [
+        ((['git', 'symbolic-ref', 'HEAD'],), 'master')
+    ] + self._gerrit_ensure_auth_calls()
+    cl = git_cl.Changelist(codereview='gerrit')
+    cl.lookedup_issue = True
+    return cl
+
+  def test_gerrit_ensure_authenticated_missing(self):
+    cl = self._test_gerrit_ensure_authenticated_common(auth={
+      'chromium.googlesource.com': 'git is ok, but gerrit one is missing',
+    })
+    self.calls.append(
+        ((['DieWithError',
+           'Credentials for the following hosts are required:\n'
+           '  chromium-review.googlesource.com\n'
+           'These are read from ~/.gitcookies (or legacy ~/.netrc)\n'
+           'You can (re)generate your credentails by visiting '
+           'https://chromium-review.googlesource.com/new-password'],), ''),)
+    self.assertIsNone(cl.EnsureAuthenticated(force=False))
+
+  def test_gerrit_ensure_authenticated_conflict(self):
+    cl = self._test_gerrit_ensure_authenticated_common(auth={
+      'chromium.googlesource.com': 'one',
+      'chromium-review.googlesource.com': 'other',
+    })
+    self.calls.append(
+        ((['ask_for_data', 'If you know what you are doing, '
+				   								 'press Enter to continue, Ctrl+C to abort.'],), ''))
+    self.assertIsNone(cl.EnsureAuthenticated(force=False))
+
+  def test_gerrit_ensure_authenticated_ok(self):
+    cl = self._test_gerrit_ensure_authenticated_common(auth={
+      'chromium.googlesource.com': 'same',
+      'chromium-review.googlesource.com': 'same',
+    })
+    self.assertIsNone(cl.EnsureAuthenticated(force=False))
+
 
 if __name__ == '__main__':
   git_cl.logging.basicConfig(