From d4834ddd985a52e9295b39cbac33037ab2b4edca Mon Sep 17 00:00:00 2001 From: GareArc Date: Thu, 7 May 2026 02:30:35 -0700 Subject: [PATCH] fix(tools): scope builtin tool default-credential clear to tenant When two workspace members each set their own credential as the default for the same provider, both rows ended up with is_default=True for the same (tenant_id, provider). The clear UPDATE was filtered by user_id, so it only reset the caller's own defaults and left other members' defaults intact. The consumer (`get_builtin_provider`) is tenant-scoped and picks an arbitrary winner via the created_at tiebreak when multiple defaults exist, producing user-visible inconsistencies. Drop user_id from the clear filter so default selection becomes properly tenant-scoped, matching `DatasourceProviderService.set_default_datasource_provider`. Adds a regression test that asserts the clear filter_by call uses tenant_id/provider but not user_id. Backport of equivalent fix on main to lts/1.13.x. --- .../tools/builtin_tools_manage_service.py | 4 ++-- .../test_builtin_tools_manage_service.py | 21 +++++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/api/services/tools/builtin_tools_manage_service.py b/api/services/tools/builtin_tools_manage_service.py index 6797a67dde..9c39eb993b 100644 --- a/api/services/tools/builtin_tools_manage_service.py +++ b/api/services/tools/builtin_tools_manage_service.py @@ -416,9 +416,9 @@ class BuiltinToolManageService: if target_provider is None: raise ValueError("provider not found") - # clear default provider + # clear default provider (tenant-scoped: only one default per provider per workspace) session.query(BuiltinToolProvider).filter_by( - tenant_id=tenant_id, user_id=user_id, provider=provider, is_default=True + tenant_id=tenant_id, provider=provider, is_default=True ).update({"is_default": False}) # set new default provider diff --git a/api/tests/unit_tests/services/tools/test_builtin_tools_manage_service.py b/api/tests/unit_tests/services/tools/test_builtin_tools_manage_service.py index 439d203c58..5bc8c231d6 100644 --- a/api/tests/unit_tests/services/tools/test_builtin_tools_manage_service.py +++ b/api/tests/unit_tests/services/tools/test_builtin_tools_manage_service.py @@ -189,6 +189,27 @@ class TestSetDefaultProvider: assert target.is_default is True session.commit.assert_called_once() + @patch(f"{MODULE}.Session") + @patch(f"{MODULE}.db") + def test_clear_default_is_tenant_scoped_not_user_scoped(self, mock_db, mock_session_cls): + # Regression: clearing prior defaults must NOT filter by user_id, otherwise + # two workspace members can each leave their own credential as default at + # the same time (the default flag is tenant-scoped, not per-user). + session = _mock_session(mock_session_cls) + session.query.return_value.filter_by.return_value.first.return_value = MagicMock() + + BuiltinToolManageService.set_default_provider("tenant-1", "user-A", "google", "cred-id") + + clear_calls = [ + call + for call in session.query.return_value.filter_by.call_args_list + if call.kwargs.get("is_default") is True + ] + assert len(clear_calls) == 1 + assert "user_id" not in clear_calls[0].kwargs + assert clear_calls[0].kwargs["tenant_id"] == "tenant-1" + assert clear_calls[0].kwargs["provider"] == "google" + class TestUpdateBuiltinToolProvider: @patch(f"{MODULE}.Session")