langgenius/dify
1
0
Fork 0
mirror of https://github.com/langgenius/dify.git synced 2026-07-21 12:05:30 -04:00
Code Issues 775 Packages Projects Releases 10 Wiki Activity

[PR #30242] fix(api): fix credential type handling and rebuild subscription transaction safety #32742

New Issue
Closed
opened 2026-02-21 20:51:59 -05:00 by yindo · 0 comments
yindo commented 2026-02-21 20:51:59 -05:00
Owner
Copy Link
Copy Source

Original Pull Request: https://github.com/langgenius/dify/pull/30242

State: closed
Merged: Yes

Important

  1. Make sure you have read our contribution guidelines
  2. Ensure there is an associated issue and you have been assigned to it
  3. Use the correct syntax to link this PR: Fixes #<issue number>.

Summary

Fix #30241

This PR fixes two critical bugs in the trigger provider service that affect credential handling and data integrity:

Issue 1: Hardcoded Credential Type in Plugin Parameter Service

Problem: The get_dynamic_select_options_with_credentials method in plugin_parameter_service.py hardcoded CredentialType.API_KEY.value instead of using the actual credential type from the subscription, causing incorrect behavior for OAuth2 subscriptions.

Solution: Replace the hardcoded value with original_subscription.credential_type or CredentialType.UNAUTHORIZED.value to respect different credential types.

Issue 2: Missing Credential Merging and Transaction Safety in rebuild_trigger_subscription

Problem: The rebuild_trigger_subscription method had several critical issues:

  • Did not merge incoming credentials with existing ones (HIDDEN_VALUE was not honored)
  • Performed unsubscribe, subscribe, and update as separate operations without a single DB transaction
  • No rollback mechanism, causing possible data loss or inconsistency if any operation failed
  • Used separate database sessions for each operation

Solution:

  • Add credential merging logic: for each credential field, if caller passed HIDDEN_VALUE, retain the existing decrypted value from subscription.credentials; otherwise use the provided value
  • Wrap all database operations (unsubscribe, subscribe, update) in a single Session transaction
  • Inline update logic to reuse the same session for consistency
  • Add proper error handling with rollback on failure
  • Use merged and properly encrypted credentials for both subscribe and update operations

Testing

Added comprehensive integration unit tests with 10 test cases covering:

  • Credential merging with HIDDEN_VALUE handling
  • Single transaction wrapping
  • Error handling and rollback
  • Various edge cases and error conditions

Files Added:

  • api/tests/test_containers_integration_tests/services/test_trigger_provider_service.py (682 lines, 10 test cases, all passing)

Test Results: All 10 tests pass successfully ✅

Motivation and Context

These bugs could lead to:

  • Incorrect credential type handling for OAuth2 subscriptions
  • Data loss or inconsistency during subscription rebuild operations
  • Security issues if credentials are not properly merged and encrypted

The fixes ensure:

  • Correct credential type handling for all subscription types
  • Data integrity through transaction safety
  • Proper credential merging to preserve existing values when needed

Dependencies

No new dependencies required. This PR only modifies existing code and adds tests.

Screenshots

N/A - Backend code changes only, no UI changes.

Checklist

  • This change requires a documentation update, included: Dify Document
  • I understand that this PR may be closed in case there was no previous discussion or issues. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.
  • I ran dev/reformat(backend) and cd web && npx lint-staged(frontend) to appease the lint gods

Additional Notes

Code Quality

  • All code follows existing patterns and conventions
  • Proper error handling with rollback mechanism
  • Comprehensive test coverage (10 integration tests)
  • Code formatting applied with ./dev/reformat
  • No linting errors

Testing Details

  • Test file: api/tests/test_containers_integration_tests/services/test_trigger_provider_service.py
  • Test execution: All 10 tests pass
  • Test coverage includes:
    • Success scenarios with credential merging
    • Error handling and rollback scenarios
    • Edge cases (missing keys, all HIDDEN_VALUE, etc.)
    • Error conditions (subscription not found, provider not found, etc.)

Breaking Changes

None - This is a bug fix that maintains backward compatibility.

**Original Pull Request:** https://github.com/langgenius/dify/pull/30242 **State:** closed **Merged:** Yes --- > [!IMPORTANT] > > 1. Make sure you have read our [contribution guidelines](https://github.com/langgenius/dify/blob/main/CONTRIBUTING.md) > 1. Ensure there is an associated issue and you have been assigned to it > 1. Use the correct syntax to link this PR: `Fixes #<issue number>`. ## Summary Fix #30241 This PR fixes two critical bugs in the trigger provider service that affect credential handling and data integrity: ### Issue 1: Hardcoded Credential Type in Plugin Parameter Service **Problem**: The `get_dynamic_select_options_with_credentials` method in `plugin_parameter_service.py` hardcoded `CredentialType.API_KEY.value` instead of using the actual credential type from the subscription, causing incorrect behavior for OAuth2 subscriptions. **Solution**: Replace the hardcoded value with `original_subscription.credential_type or CredentialType.UNAUTHORIZED.value` to respect different credential types. ### Issue 2: Missing Credential Merging and Transaction Safety in rebuild_trigger_subscription **Problem**: The `rebuild_trigger_subscription` method had several critical issues: - Did not merge incoming credentials with existing ones (HIDDEN_VALUE was not honored) - Performed unsubscribe, subscribe, and update as separate operations without a single DB transaction - No rollback mechanism, causing possible data loss or inconsistency if any operation failed - Used separate database sessions for each operation **Solution**: - Add credential merging logic: for each credential field, if caller passed `HIDDEN_VALUE`, retain the existing decrypted value from `subscription.credentials`; otherwise use the provided value - Wrap all database operations (unsubscribe, subscribe, update) in a single `Session` transaction - Inline update logic to reuse the same session for consistency - Add proper error handling with rollback on failure - Use merged and properly encrypted credentials for both subscribe and update operations ### Testing Added comprehensive integration unit tests with 10 test cases covering: - Credential merging with HIDDEN_VALUE handling - Single transaction wrapping - Error handling and rollback - Various edge cases and error conditions **Files Added**: - `api/tests/test_containers_integration_tests/services/test_trigger_provider_service.py` (682 lines, 10 test cases, all passing) **Test Results**: All 10 tests pass successfully ✅ ### Motivation and Context These bugs could lead to: - Incorrect credential type handling for OAuth2 subscriptions - Data loss or inconsistency during subscription rebuild operations - Security issues if credentials are not properly merged and encrypted The fixes ensure: - Correct credential type handling for all subscription types - Data integrity through transaction safety - Proper credential merging to preserve existing values when needed ### Dependencies No new dependencies required. This PR only modifies existing code and adds tests. ## Screenshots N/A - Backend code changes only, no UI changes. ## Checklist - [ ] This change requires a documentation update, included: [Dify Document](https://github.com/langgenius/dify-docs) - [x] I understand that this PR may be closed in case there was no previous discussion or issues. (This doesn't apply to typos!) - [x] I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change. - [x] I've updated the documentation accordingly. - [x] I ran `dev/reformat`(backend) and `cd web && npx lint-staged`(frontend) to appease the lint gods ## Additional Notes ### Code Quality - All code follows existing patterns and conventions - Proper error handling with rollback mechanism - Comprehensive test coverage (10 integration tests) - Code formatting applied with `./dev/reformat` - No linting errors ### Testing Details - Test file: `api/tests/test_containers_integration_tests/services/test_trigger_provider_service.py` - Test execution: All 10 tests pass - Test coverage includes: - Success scenarios with credential merging - Error handling and rollback scenarios - Edge cases (missing keys, all HIDDEN_VALUE, etc.) - Error conditions (subscription not found, provider not found, etc.) ### Breaking Changes None - This is a bug fix that maintains backward compatibility.
yindo added the pull-request label 2026-02-21 20:51:59 -05:00
yindo closed this issue 2026-02-21 20:51:59 -05:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
Knowledge Pipeline
Proposal
WIP
cloud
dependencies
feat:webapp
good first issue
outdated
performance
pull-request
Mirrored from GitHub Pull Request
 python:uv
refactor
size:L
stale
status: accepting prs
status: blocked
status: in discussion
stt & tts
vulnerablity
web
wont-fix
🌊 feat:workflow
🌚 invalid
🐞 bug
👻 feat:rag
💪 enhancement
📚 documentation
🔨 feat:tools
🙋‍♂️ question
🙏 help wanted
🤔 cant-reproduce
🤖 feat:agent
No Label pull-request
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
yindo
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: langgenius/dify#32742
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?