fix: clear all default providers in tenant for enterprise mode #31567
Conversation
… only) When setting a new default credential in enterprise mode, the code was only clearing is_default for credentials matching the current user_id. This caused issues when: 1. Enterprise credential A (synced with system user_id) was default 2. User sets local credential B as default 3. A still had is_default=true (different user_id) 4. Both A and B were considered defaults The fix removes user_id from the filter only for enterprise deployments, since enterprise credentials may have different user_id than local ones. Non-enterprise behavior is unchanged to avoid breaking existing setups. Fixes EE-1511
dosubot
bot
added
the
size:S
Summary of ChangesHello @GareArc, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in enterprise mode where setting a new default tool credential could fail to properly clear all previous defaults for the same provider. The issue stemmed from Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses the issue of multiple default tool credentials in enterprise mode by introducing conditional logic to clear all existing defaults for a given provider within a tenant when dify_config.ENTERPRISE_ENABLED is true. However, the set_default_provider function introduces an IDOR vulnerability due to a missing tenant-scoping check when fetching the target provider by ID. Furthermore, the new Enterprise logic allows any user to modify tenant-wide settings without administrative authorization.
- Add tenant_id verification to prevent IDOR attacks - Add admin check for enterprise tenant-wide default changes - Preserve non-enterprise behavior (users can set own defaults)
dosubot
bot
added
size:M
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request aims to address the issue of incorrect default tool credential clearing in enterprise mode (EE-1511) by differentiating behavior between enterprise and non-enterprise modes and ensuring all existing defaults for a provider within a tenant are cleared. It also includes crucial security enhancements, such as verifying tenant ownership and administrative permission checks. While it fixes an IDOR vulnerability in set_default_provider, a critical logic flaw remains: the provider name from the request is not validated against the credential's provider attribute. This oversight could allow users to bypass the intended clearing logic, leading to multiple default credentials for a single provider. Additionally, the type hinting for the Account model is a good improvement for code clarity.
| if account and not TenantAccountRole.is_privileged_role(account.current_role): | ||
| raise ValueError("Only workspace admins/owners can set default credentials in enterprise mode") |
There was a problem hiding this comment.
The permission check if account and not TenantAccountRole.is_privileged_role(account.current_role): is a high-priority security measure. It correctly restricts the ability to set tenant-wide default credentials to only privileged roles (admins/owners) in enterprise mode, preventing unauthorized changes.
| # get provider | ||
| target_provider = session.query(BuiltinToolProvider).filter_by(id=id).first() | ||
| # get provider (verify tenant ownership to prevent IDOR) | ||
| target_provider = session.query(BuiltinToolProvider).filter_by(id=id, tenant_id=tenant_id).first() |
There was a problem hiding this comment.
The set_default_provider method currently lacks validation to ensure the provider name from the request matches the provider attribute of the credential being updated. This logic flaw could allow an attacker to bypass the intended clearing mechanism and create multiple default credentials for a single provider, leading to unpredictable behavior. While the inclusion of tenant_id=tenant_id correctly prevents IDOR vulnerabilities, the provider validation is still crucial for maintaining data integrity and preventing unintended credential assignments.
# get provider (verify tenant ownership and provider match to prevent IDOR and logic flaws)
target_provider = session.query(BuiltinToolProvider).filter_by(id=id, tenant_id=tenant_id, provider=provider).first()| session.query(BuiltinToolProvider).filter_by( | ||
| tenant_id=tenant_id, provider=provider, is_default=True | ||
| ).update({"is_default": False}) |
There was a problem hiding this comment.
This change correctly implements the core logic for enterprise mode, clearing all is_default=True flags for the specified provider within the tenant. This directly resolves the problem described in EE-1511, where credentials synced from global might have different user_ids, leading to multiple active defaults.
Summary
Problem
In enterprise mode, credentials synced from global have a different
user_idthan locally-created credentials. The existing code only cleared defaults for credentials with the sameuser_id, resulting in multiple credentials havingis_default=truesimultaneously.This caused the credential usage check to incorrectly show workflows as "using" a credential that was no longer the effective default.
Solution
is_default=truefor the provider in the tenant (regardless ofuser_id)Related