From 6da6aecbb4213d99d41ff67eac13c2d1e23e23c9 Mon Sep 17 00:00:00 2001 From: Jiewei Qian Date: Tue, 16 Sep 2025 17:00:01 -0700 Subject: [PATCH] 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 Reviewed-by: Scott Lee Commit-Queue: Jiewei Qian --- gerrit_util.py | 21 ++++++++++++++-- git_cl.py | 6 +++-- tests/gerrit_util_test.py | 53 +++++++++++++++++++++++++++++++++++++++ tests/git_cl_test.py | 11 +++++--- 4 files changed, 83 insertions(+), 8 deletions(-) diff --git a/gerrit_util.py b/gerrit_util.py index 6fc66fb813..603a12dd28 100644 --- a/gerrit_util.py +++ b/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(): diff --git a/git_cl.py b/git_cl.py index 8e45c1161c..e465596914 100755 --- a/git_cl.py +++ b/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. diff --git a/tests/gerrit_util_test.py b/tests/gerrit_util_test.py index 512386b181..874e96e695 100755 --- a/tests/gerrit_util_test.py +++ b/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): diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 149f37840a..bb706ffa6c 100755 --- a/tests/git_cl_test.py +++ b/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']))