git-cl: make SetReview ReAuth capable

This CL changes SetReview to be ReAuth capable.

This is a potential breaking change if an downstream script relies on
gerrit_util.SetReview but hasn't implemented ReAuth.

If this caused breakage, please refer to https://chromium.googlesource.com/chromium/src.git/+/HEAD/docs/gerrit_reauth.md#Troubleshooting for troubleshooting instructions.

If the above doesn't work, please report a bug, then add `LUCI_BYPASS_REAUTH=1` to the environment to disable ReAuth for now.

Bug: 442666611
Change-Id: I7724f15f166f9975fc88be5d8e1c93be4e1ec302
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6939308
Reviewed-by: Allen Li <ayatane@chromium.org>
Reviewed-by: Scott Lee <ddoman@chromium.org>
Commit-Queue: Jiewei Qian <qjw@chromium.org>

gerrit_util.py

@@ -1766,10 +1766,15 @@ def SetReview(host,
               labels=None,
               notify=None,
               ready=None,
-              automatic_attention_set_update: Optional[bool] = None):
+              automatic_attention_set_update: Optional[bool] = None,
+              project: Optional[str] = None):
     """Sets labels and/or adds a message to a code review.
 
     https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#set-review
+
+    We need to check if this change's `project` needs to satisfy ReAuth
+    requirements, and attach an ReAuth credential to the request. `project`
+    can be optionally be provided to save one Gerrit server round-trip.
     """
     if not msg and not labels:
         return
@@ -1786,7 +1791,19 @@ def SetReview(host,
     if automatic_attention_set_update is not None:
         body[
             'ignore_automatic_attention_set_rules'] = not automatic_attention_set_update
-    conn = CreateHttpConn(host, path, reqtype='POST', body=body)
+
+    # ReAuth needed if we set labels (notably Code-Review).
+    reauth_context = None
+    if bool(labels):
+        reauth_context = auth.ReAuthContext(
+            host=host,
+            project=project or GetChangeDetail(host, change)["project"])
+
+    conn = CreateHttpConn(host,
+                          path,
+                          reqtype='POST',
+                          body=body,
+                          reauth_context=reauth_context)
     response = ReadHttpJsonResponse(conn)
     if labels:
         for key, val in labels.items():

git_cl.py

@@ -2270,7 +2270,8 @@ class Changelist(object):
             gerrit_util.SetReview(self.GetGerritHost(),
                                   self._GerritChangeIdentifier(),
                                   labels=labels,
-                                  notify=notify)
+                                  notify=notify,
+                                  project=self.GetGerritProject())
             return 0
         except KeyboardInterrupt:
             raise
@@ -2588,7 +2589,8 @@ class Changelist(object):
         gerrit_util.SetReview(self.GetGerritHost(),
                               self._GerritChangeIdentifier(),
                               msg=message,
-                              ready=publish)
+                              ready=publish,
+                              project=self.GetGerritProject())
 
     def GetCommentsSummary(self, readable=True):
         # DETAILED_ACCOUNTS is to get emails in accounts.

tests/gerrit_util_test.py

@@ -656,6 +656,59 @@ class GerritUtilTest(unittest.TestCase):
         mockJsonResponse.return_value = {'status': {}}
         self.assertTrue(gerrit_util.IsCodeOwnersEnabledOnRepo('host', 'repo'))
 
+    @mock.patch('gerrit_util.CreateHttpConn')
+    @mock.patch('gerrit_util.ReadHttpJsonResponse')
+    @mock.patch('gerrit_util.GetChangeDetail')
+    def testSetReview_ReAuth(self, mockGetChangeDetail, mockJsonResponse,
+                             mockCreateHttpConn):
+        mockJsonResponse.return_value = {"labels": {"Code-Review": 1}}
+        mockGetChangeDetail.return_value = {
+            "project": "infra/infra",
+        }
+
+        gerrit_util.SetReview('chromium', 123456, labels={'Code-Review': 1})
+
+        # Check `project` in reauth_context is backfilled.
+        mockGetChangeDetail.assert_called_with('chromium', 123456)
+        httpConnKwargs = mockCreateHttpConn.call_args[1]
+        self.assertIn('reauth_context', httpConnKwargs)
+        self.assertEqual(
+            auth.ReAuthContext(host='chromium', project='infra/infra'),
+            httpConnKwargs['reauth_context'])
+
+    @mock.patch('gerrit_util.CreateHttpConn')
+    @mock.patch('gerrit_util.ReadHttpJsonResponse')
+    @mock.patch('gerrit_util.GetChangeDetail')
+    def testSetReview_ReAuth_WithProject(self, mockGetChangeDetail,
+                                         mockJsonResponse, mockCreateHttpConn):
+        mockJsonResponse.return_value = {"labels": {"Code-Review": 1}}
+
+        gerrit_util.SetReview('chromium',
+                              123456,
+                              labels={'Code-Review': 1},
+                              project="infra/experimental")
+
+        # Check reauth_context uses the given project.
+        mockGetChangeDetail.assert_not_called()
+        httpConnKwargs = mockCreateHttpConn.call_args[1]
+        self.assertIn('reauth_context', httpConnKwargs)
+        self.assertEqual(
+            auth.ReAuthContext(host='chromium', project='infra/experimental'),
+            httpConnKwargs['reauth_context'])
+
+    @mock.patch('gerrit_util.CreateHttpConn')
+    @mock.patch('gerrit_util.ReadHttpJsonResponse')
+    @mock.patch('gerrit_util.GetChangeDetail')
+    def testSetReview_ReAuthNotNeeded(self, mockGetChangeDetail,
+                                      mockJsonResponse, mockCreateHttpConn):
+        mockJsonResponse.return_value = {"ready": True}
+
+        gerrit_util.SetReview('chromium', 123456, msg="test")
+
+        # ReAuth not needed.
+        mockGetChangeDetail.assert_not_called()
+        httpConnKwargs = mockCreateHttpConn.call_args[1]
+        self.assertIsNone(httpConnKwargs.get('reauth_context', None))
 
 class SSOAuthenticatorTest(unittest.TestCase):
 

tests/git_cl_test.py

@@ -638,9 +638,11 @@ class TestGitCl(unittest.TestCase):
         mock.patch('git_cl.gerrit_util.AddReviewers',
                    lambda *a: self._mocked_call('AddReviewers', *a)).start()
         mock.patch('git_cl.gerrit_util.SetReview',
-                   lambda h, i, msg=None, labels=None, notify=None, ready=None:
+                   lambda h, i, msg=None, labels=None, notify=None, ready=None,
+                   automatic_attention_set_update=None, project=None:
                    (self._mocked_call('SetReview', h, i, msg, labels, notify,
-                                      ready))).start()
+                                      ready, automatic_attention_set_update,
+                                      project))).start()
         mock.patch('git_cl.gerrit_util.LuciContextAuthenticator.is_applicable',
                    return_value=False).start()
         mock.patch('git_cl.gerrit_util.GceAuthenticator.is_applicable',
@@ -2551,7 +2553,7 @@ class TestGitCl(unittest.TestCase):
             (('SetReview', 'chromium-review.googlesource.com',
               'infra%2Finfra~123', None, {
                   'Commit-Queue': vote
-              }, notify, None), ''),
+              }, notify, None, None, 'infra/infra'), ''),
         ]
 
     @unittest.skipIf(gclient_utils.IsEnvCog(),
@@ -3205,7 +3207,8 @@ class TestGitCl(unittest.TestCase):
                           'https://chromium.googlesource.com/infra/infra')
         self.calls = [
             (('SetReview', 'chromium-review.googlesource.com',
-              'infra%2Finfra~10', 'msg', None, None, None), None),
+              'infra%2Finfra~10', 'msg', None, None, None, None, 'infra/infra'),
+             None),
         ]
         self.assertEqual(0, git_cl.main(['comment', '-i', '10', '-a', 'msg']))