From dc8d502d13ca2cc6046cbe20ff0064193a7dff3d Mon Sep 17 00:00:00 2001 From: Allen Li Date: Wed, 4 Sep 2024 21:18:53 +0000 Subject: [PATCH] [gerrit_util] Don't try to use SSO on non-googlesource hosts In case you're getting funny thoughts, these requests would not attach any cookies anyway because the domain doesn't match. It just allows requests to properly fallback to OAuth path. Bug: b/362741558 Change-Id: Iaf83ad640501ff45671dbc358e676cbeaf04d686 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5824222 Commit-Queue: Allen Li Reviewed-by: Josip Sokcevic --- gerrit_util.py | 3 +++ tests/gerrit_util_test.py | 34 +++++++++++++++++++++++++--------- 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/gerrit_util.py b/gerrit_util.py index b5dd326ce..3b9470c98 100644 --- a/gerrit_util.py +++ b/gerrit_util.py @@ -179,6 +179,9 @@ def ShouldUseSSO(host: str, email: str) -> bool: if not newauth.Enabled(): LOGGER.debug("SSO=False: not opted in") return False + if not host.endswith('.googlesource.com'): + LOGGER.debug("SSO=False: non-googlesource host %r", host) + return False if newauth.SkipSSO(): LOGGER.debug("SSO=False: set skip SSO config") return False diff --git a/tests/gerrit_util_test.py b/tests/gerrit_util_test.py index 5273d5a33..4d92842f2 100755 --- a/tests/gerrit_util_test.py +++ b/tests/gerrit_util_test.py @@ -752,23 +752,33 @@ class ShouldUseSSOTest(unittest.TestCase): @mock.patch('newauth.Enabled', return_value=False) def testDisabled(self, _): self.assertFalse( - gerrit_util.ShouldUseSSO('fake-host', 'firefly@google.com')) + gerrit_util.ShouldUseSSO('fake-host.googlesource.com', + 'firefly@google.com')) @mock.patch('gerrit_util.ssoHelper.find_cmd', return_value='') def testMissingCommand(self, _): self.assertFalse( - gerrit_util.ShouldUseSSO('fake-host', 'firefly@google.com')) + gerrit_util.ShouldUseSSO('fake-host.googlesource.com', + 'firefly@google.com')) + + def testBadHost(self): + self.assertFalse( + gerrit_util.ShouldUseSSO('fake-host.coreboot.org', + 'firefly@google.com')) def testEmptyEmail(self): - self.assertTrue(gerrit_util.ShouldUseSSO('fake-host', '')) + self.assertTrue( + gerrit_util.ShouldUseSSO('fake-host.googlesource.com', '')) def testGoogleEmail(self): self.assertTrue( - gerrit_util.ShouldUseSSO('fake-host', 'firefly@google.com')) + gerrit_util.ShouldUseSSO('fake-host.googlesource.com', + 'firefly@google.com')) def testGmail(self): self.assertFalse( - gerrit_util.ShouldUseSSO('fake-host', 'firefly@gmail.com')) + gerrit_util.ShouldUseSSO('fake-host.googlesource.com', + 'firefly@gmail.com')) @mock.patch('gerrit_util.GetAccountEmails', return_value=[{ @@ -776,8 +786,11 @@ class ShouldUseSSOTest(unittest.TestCase): }]) def testLinkedChromium(self, email): self.assertTrue( - gerrit_util.ShouldUseSSO('fake-host', 'firefly@chromium.org')) - email.assert_called_with('fake-host', 'self', authenticator=mock.ANY) + gerrit_util.ShouldUseSSO('fake-host.googlesource.com', + 'firefly@chromium.org')) + email.assert_called_with('fake-host.googlesource.com', + 'self', + authenticator=mock.ANY) @mock.patch('gerrit_util.GetAccountEmails', return_value=[{ @@ -785,8 +798,11 @@ class ShouldUseSSOTest(unittest.TestCase): }]) def testUnlinkedChromium(self, email): self.assertFalse( - gerrit_util.ShouldUseSSO('fake-host', 'firefly@chromium.org')) - email.assert_called_with('fake-host', 'self', authenticator=mock.ANY) + gerrit_util.ShouldUseSSO('fake-host.googlesource.com', + 'firefly@chromium.org')) + email.assert_called_with('fake-host.googlesource.com', + 'self', + authenticator=mock.ANY) if __name__ == '__main__':