diff --git a/gerrit_util.py b/gerrit_util.py index acaacb2895..53dc69c0c6 100755 --- a/gerrit_util.py +++ b/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. diff --git a/git_cl.py b/git_cl.py index d9982107b5..37d0523fc4 100755 --- a/git_cl.py +++ b/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.""" diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index a9c1072dd5..eca9cf4fb8 100755 --- a/tests/git_cl_test.py +++ b/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(