Skip to content

fix(tools): scope builtin tool default-credential clear to tenant#35887

Merged
GareArc merged 3 commits into
mainfrom
fix/builtin-tool-default-credential-main
May 8, 2026
Merged

fix(tools): scope builtin tool default-credential clear to tenant#35887
GareArc merged 3 commits into
mainfrom
fix/builtin-tool-default-credential-main

Conversation

@GareArc
Copy link
Copy Markdown
Contributor

@GareArc GareArc commented May 7, 2026

Summary

  • Fixes a bug where two workspace members each setting their own credential as default for the same builtin tool provider left both rows with is_default=True for the same (tenant_id, provider).
  • The clear-default UPDATE in BuiltinToolManageService.set_default_provider was filtered by user_id, so it only reset the caller's own defaults. The consumer (get_builtin_provider) is tenant-scoped and breaks ties via created_at, producing user-visible inconsistencies (UI shows credential B as default while runtime uses credential A).
  • Drops user_id from the clear filter, mirroring DatasourceProviderService.set_default_datasource_provider. The is_default flag is now correctly tenant-scoped (one default per provider per workspace).

Reproduction

  1. Member A in workspace W creates cred1 (provider P), sets it as default
  2. Member B in workspace W creates cred2 (provider P), sets it as default
  3. Both rows now have is_default=True — DB has two defaults for the same (tenant_id, provider)

Reproduced on production and on console-platform.dify.dev.

Test plan

  • Unit test added: TestSetDefaultProvider::test_clear_default_is_tenant_scoped_not_user_scoped — asserts the compiled UPDATE WHERE clause contains tenant_id/provider but not user_id. Verified failing on pre-fix code, passing on fixed code.
  • Existing TestSetDefaultProvider tests still pass.
  • Manual reproduction on staging once merged.

Notes

  • Pre-existing TOCTOU race between concurrent transactions remains (two callers for the same tenant could still produce two is_default=True rows in a narrow window). A partial unique index UNIQUE (tenant_id, provider) WHERE is_default would close this — out of scope here, flagging for follow-up.
  • Equivalent fix backported to lts/1.13.x in a separate PR.
  • user_id parameter retained on set_default_provider signature: still passed by the controller via kwargs and may be used by EE callers; removing it would be a breaking change for kwarg callers.

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 compiled UPDATE WHERE clause
contains tenant_id/provider but not user_id.
@dosubot dosubot Bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label May 7, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

Pyrefly Type Coverage

Metric Base PR Delta
Type coverage 0.00% 41.39% +41.39%
Strict coverage 0.00% 40.91% +40.91%
Typed symbols 0 20,728 +20,728
Untyped symbols 0 29,697 +29,697
Modules 0 2536 +2,536

@codecov
Copy link
Copy Markdown

codecov Bot commented May 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.81%. Comparing base (1b0d463) to head (2c6d90b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #35887      +/-   ##
==========================================
+ Coverage   85.80%   85.81%   +0.01%     
==========================================
  Files        4456     4462       +6     
  Lines      208899   208769     -130     
  Branches    39075    38961     -114     
==========================================
- Hits       179242   179159      -83     
+ Misses      26482    26434      -48     
- Partials     3175     3176       +1     
Flag Coverage Δ
api 85.11% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@GareArc GareArc requested a review from wylswz May 8, 2026 00:32
Comment thread api/services/tools/builtin_tools_manage_service.py Outdated
@GareArc GareArc requested a review from wylswz May 8, 2026 03:27
@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label May 8, 2026
@wylswz wylswz added this pull request to the merge queue May 8, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 8, 2026
@GareArc GareArc enabled auto-merge May 8, 2026 04:59
@GareArc GareArc added this pull request to the merge queue May 8, 2026
Merged via the queue into main with commit 29f3484 May 8, 2026
32 checks passed
@GareArc GareArc deleted the fix/builtin-tool-default-credential-main branch May 8, 2026 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants