git cl upload for Gerrit: warn if uploader is not CL owner.

R=agable@chromium.org
BUG=653172

Change-Id: I141a1422a8f526040fe80522a41d14e2cd8cfc07
Reviewed-on: https://chromium-review.googlesource.com/458421
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>
Reviewed-by: Aaron Gable <agable@chromium.org>

gerrit_util.py

@@ -184,19 +184,30 @@ class CookiesAuthenticator(Authenticator):
 
     return gitcookies
 
-  def get_auth_header(self, host):
-    auth = None
+  def _get_auth_for_host(self, host):
     for domain, creds in self.gitcookies.iteritems():
       if cookielib.domain_match(host, domain):
-        auth = (creds[0], None, creds[1])
-        break
+        return (creds[0], None, creds[1])
+    return self.netrc.authenticators(host)
 
-    if not auth:
-      auth = self.netrc.authenticators(host)
+  def get_auth_header(self, host):
+    auth = self._get_auth_for_host(host)
     if auth:
       return 'Basic %s' % (base64.b64encode('%s:%s' % (auth[0], auth[2])))
     return None
 
+  def get_auth_email(self, host):
+    """Best effort parsing of email to be used for auth for the given host."""
+    auth = self._get_auth_for_host(host)
+    if not auth:
+      return None
+    login = auth[0]
+    # login typically looks like 'git-xxx.example.com'
+    if not login.startswith('git-') or '.' not in login:
+      return None
+    username, domain = login[len('git-'):].split('.', 1)
+    return '%s@%s' % (username, domain)
+
 
 # Backwards compatibility just in case somebody imports this outside of
 # depot_tools.
@@ -805,6 +816,21 @@ def GetGerritBranch(host, project, branch):
   raise GerritError(200, 'Unable to get gerrit branch')
 
 
+def GetAccountDetails(host, account_id='self'):
+  """Returns details of the account.
+
+  If account_id is not given, uses magic value 'self' which corresponds to
+  whichever account user is authenticating as.
+
+  Documentation:
+    https://gerrit-review.googlesource.com/Documentation/rest-api-accounts.html#get-account
+  """
+  if account_id != 'self':
+    account_id = int(account_id)
+  conn = CreateHttpConn(host, '/accounts/%s' % account_id)
+  return ReadHttpJsonResponse(conn)
+
+
 @contextlib.contextmanager
 def tempdir():
   tdir = None

git_cl.py

@@ -1540,7 +1540,7 @@ class Changelist(object):
     # expensive hooks if uploading is likely to fail anyway. Passing these
     # checks does not guarantee that uploading will not fail.
     self._codereview_impl.EnsureAuthenticated(force=options.force)
-    self._codereview_impl.EnsureCanUploadPatchset()
+    self._codereview_impl.EnsureCanUploadPatchset(force=options.force)
 
     # Apply watchlists on upload.
     change = self.GetChange(base_branch, None)
@@ -1809,14 +1809,17 @@ class _ChangelistCodereviewBase(object):
     """
     raise NotImplementedError()
 
-  def EnsureCanUploadPatchset(self):
+  def EnsureCanUploadPatchset(self, force):
     """Best effort check that uploading isn't supposed to fail for predictable
     reasons.
 
     This method should raise informative exception if uploading shouldn't
     proceed.
+
+    Arguments:
+      force: whether to skip confirmation questions.
     """
-    pass
+    raise NotImplementedError()
 
   def CMDUploadChange(self, options, args, change):
     """Uploads a change to codereview."""
@@ -1873,6 +1876,10 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase):
       if refresh:
         authenticator.get_access_token()
 
+  def EnsureCanUploadPatchset(self, force):
+    # No checks for Rietveld because we are deprecating Rietveld.
+    pass
+
   def FetchDescription(self, force=False):
     issue = self.GetIssue()
     assert issue
@@ -2383,19 +2390,14 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
                      cookie_auth.get_netrc_path(),
                      cookie_auth.get_new_password_message(git_host)))
 
-  def EnsureCanUploadPatchset(self):
-    """Best effort check that uploading isn't supposed to fail for predictable
-    reasons.
-
-    This method should raise informative exception if uploading shouldn't
-    proceed.
-    """
+  def EnsureCanUploadPatchset(self, force):
     if not self.GetIssue():
       return
 
     # Warm change details cache now to avoid RPCs later, reducing latency for
     # developers.
-    self.FetchDescription()
+    self._GetChangeDetail(
+        ['DETAILED_ACCOUNTS', 'CURRENT_REVISION', 'CURRENT_COMMIT'])
 
     status = self._GetChangeDetail()['status']
     if status in ('MERGED', 'ABANDONED'):
@@ -2403,6 +2405,27 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
                    (self.GetIssueURL(),
                     'submitted' if status == 'MERGED' else 'abandoned'))
 
+    if gerrit_util.GceAuthenticator.is_gce():
+      return
+    cookies_user = gerrit_util.CookiesAuthenticator().get_auth_email(
+        self._GetGerritHost())
+    if self.GetIssueOwner() == cookies_user:
+      return
+    logging.debug('change %s owner is %s, cookies user is %s',
+                  self.GetIssue(), self.GetIssueOwner(), cookies_user)
+    # Maybe user has linked accounts or smth like that,
+    # so ask what Gerrit thinks of this user.
+    details = gerrit_util.GetAccountDetails(self._GetGerritHost(), 'self')
+    if details['email'] == self.GetIssueOwner():
+      return
+    if not force:
+      print('WARNING: change %s is owned by %s, but you authenticate to Gerrit '
+            'as %s.\n'
+            'Uploading may fail due to lack of permissions.' %
+            (self.GetIssue(), self.GetIssueOwner(), details['email']))
+      confirm_or_exit(action='upload')
+
+
   def _PostUnsetIssueProperties(self):
     """Which branch-specific properties to erase when unsetting issue."""
     return ['gerritsquashhash']

tests/git_cl_test.py

@@ -132,16 +132,17 @@ class AuthenticatorMock(object):
     return http
 
 
-def CookiesAuthenticatorMockFactory(hosts_with_creds=None, same_cookie=False):
+def CookiesAuthenticatorMockFactory(hosts_with_creds=None, same_auth=False):
   """Use to mock Gerrit/Git credentials from ~/.netrc or ~/.gitcookies.
 
   Usage:
     >>> self.mock(git_cl.gerrit_util, "CookiesAuthenticator",
-                  CookiesAuthenticatorMockFactory({'host1': 'cookie1'}))
+                  CookiesAuthenticatorMockFactory({'host': ('user', _, 'pass')})
 
   OR
     >>> self.mock(git_cl.gerrit_util, "CookiesAuthenticator",
-                  CookiesAuthenticatorMockFactory(cookie='cookie'))
+                  CookiesAuthenticatorMockFactory(
+                      same_auth=('user', '', 'pass'))
   """
   class CookiesAuthenticatorMock(git_cl.gerrit_util.CookiesAuthenticator):
     def __init__(self):  # pylint: disable=super-init-not-called
@@ -153,9 +154,9 @@ def CookiesAuthenticatorMockFactory(hosts_with_creds=None, same_cookie=False):
     @classmethod
     def get_netrc_path(cls):
       return '~/.netrc'
-    def get_auth_header(self, host):
-      if same_cookie:
-        return same_cookie
+    def _get_auth_for_host(self, host):
+      if same_auth:
+        return same_auth
       return (hosts_with_creds or {}).get(host)
   return CookiesAuthenticatorMock
 
@@ -1215,7 +1216,7 @@ class TestGitCl(TestCase):
 
   @classmethod
   def _gerrit_base_calls(cls, issue=None, fetched_description=None,
-                         fetched_status=None):
+                         fetched_status=None, other_cl_owner=None):
     calls = [
         ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
         ((['git', 'config', 'branch.master.git-cl-similarity'],),
@@ -1242,8 +1243,10 @@ class TestGitCl(TestCase):
     if issue:
       calls += [
         (('GetChangeDetail', 'chromium-review.googlesource.com',
-          '123456', ['CURRENT_REVISION', 'CURRENT_COMMIT']),
+          '123456',
+          ['DETAILED_ACCOUNTS', 'CURRENT_REVISION', 'CURRENT_COMMIT']),
          {
+           'owner': {'email': (other_cl_owner or 'owner@example.com')},
            'change_id': '123456789',
            'current_revision': 'sha1_of_current_revision',
            'revisions': { 'sha1_of_current_revision': {
@@ -1259,6 +1262,10 @@ class TestGitCl(TestCase):
                             'allowed'), SystemExitMock()),
         ]
         return calls
+      if other_cl_owner:
+        calls += [
+          (('ask_for_data', 'Press Enter to upload, or Ctrl+C to abort'), ''),
+        ]
 
     calls += cls._git_sanity_checks('fake_ancestor_sha', 'master',
                                   get_remote_branch=False)
@@ -1457,7 +1464,8 @@ class TestGitCl(TestCase):
       issue=None,
       cc=None,
       git_mirror=None,
-      fetched_status=None):
+      fetched_status=None,
+      other_cl_owner=None):
     """Generic gerrit upload test framework."""
     if squash_mode is None:
       if '--no-squash' in upload_args:
@@ -1471,7 +1479,8 @@ class TestGitCl(TestCase):
     cc = cc or []
     self.mock(git_cl.sys, 'stdout', StringIO.StringIO())
     self.mock(git_cl.gerrit_util, 'CookiesAuthenticator',
-              CookiesAuthenticatorMockFactory(same_cookie='same_cred'))
+              CookiesAuthenticatorMockFactory(
+                same_auth=('git-owner.example.com', '', 'pass')))
     self.mock(git_cl._GerritChangelistImpl, '_GerritCommitMsgHookCheck',
               lambda _, offer_removal: None)
     self.mock(git_cl.gclient_utils, 'RunEditor',
@@ -1488,7 +1497,8 @@ class TestGitCl(TestCase):
     self.calls = self._gerrit_base_calls(
         issue=issue,
         fetched_description=description,
-        fetched_status=fetched_status)
+        fetched_status=fetched_status,
+        other_cl_owner=other_cl_owner)
     if fetched_status != 'ABANDONED':
       self.calls += self._gerrit_upload_calls(
           description, reviewers, squash,
@@ -1610,6 +1620,24 @@ class TestGitCl(TestCase):
           issue=123456,
           fetched_status='ABANDONED')
 
+  def test_gerrit_upload_squash_reupload_to_not_owned(self):
+    self.mock(git_cl.gerrit_util, 'GetAccountDetails',
+              lambda *_, **__: {'email': 'yet-another@example.com'})
+    description = 'desc\nBUG=\n\nChange-Id: 123456789'
+    self._run_gerrit_upload_test(
+          ['--squash'],
+          description,
+          [],
+          squash=True,
+          expected_upstream_ref='origin/master',
+          issue=123456,
+          other_cl_owner='other@example.com')
+    self.assertIn(
+        'WARNING: change 123456 is owned by other@example.com, but you '
+        'authenticate to Gerrit as yet-another@example.com.\n'
+        'Uploading may fail due to lack of permissions',
+        git_cl.sys.stdout.getvalue())
+
   def test_upload_branch_deps(self):
     self.mock(git_cl.sys, 'stdout', StringIO.StringIO())
     def mock_run_git(*args, **_kwargs):
@@ -2045,7 +2073,7 @@ class TestGitCl(TestCase):
 
   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',
+      'chromium.googlesource.com': ('git-is.ok', '', 'but gerrit is missing'),
     })
     self.calls.append(
         ((['DieWithError',
@@ -2059,8 +2087,10 @@ class TestGitCl(TestCase):
   def test_gerrit_ensure_authenticated_conflict(self):
     self.mock(git_cl.sys, 'stdout', StringIO.StringIO())
     cl = self._test_gerrit_ensure_authenticated_common(auth={
-      'chromium.googlesource.com': 'one',
-      'chromium-review.googlesource.com': 'other',
+      'chromium.googlesource.com':
+          ('git-one.example.com', None, 'secret1'),
+      'chromium-review.googlesource.com':
+          ('git-other.example.com', None, 'secret2'),
     })
     self.calls.append(
         (('ask_for_data', 'If you know what you are doing '
@@ -2069,8 +2099,10 @@ class TestGitCl(TestCase):
 
   def test_gerrit_ensure_authenticated_ok(self):
     cl = self._test_gerrit_ensure_authenticated_common(auth={
-      'chromium.googlesource.com': 'same',
-      'chromium-review.googlesource.com': 'same',
+      'chromium.googlesource.com':
+          ('git-same.example.com', None, 'secret'),
+      'chromium-review.googlesource.com':
+          ('git-same.example.com', None, 'secret'),
     })
     self.assertIsNone(cl.EnsureAuthenticated(force=False))