fix(rbac): add permission checks in api and use owner id for provider sync - #14104
Merged
Conversation
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughDeployment authorization now resolves flow ownership in batches, deployment listing and reconciliation carry a separate provider-owner scope, and deployment mutations authorize before provider access. Deletion paths handle owner scoping, rollback retries, and unconfirmed database row counts explicitly. ChangesDeployment ownership and authorization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant DeploymentRoute
participant FlowDeployGuard
participant DeploymentCRUD
participant Provider
Client->>DeploymentRoute: create or update deployment
DeploymentRoute->>FlowDeployGuard: authorize flow-version deployment
FlowDeployGuard-->>DeploymentRoute: allow or deny
DeploymentRoute->>DeploymentCRUD: resolve owner-scoped deployment data
DeploymentRoute->>Provider: mutate provider resource
Provider-->>DeploymentRoute: provider result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
HzaRashid
changed the title
HzaRashid
changed the title
Contributor
|
✅ Migration Validation Passed All migrations follow the Expand-Contract pattern correctly. |
github-actions
Bot
added
bug
erichare
self-requested a review
July 15, 2026 22:31
…cess gaps - Pass row_owner_id through list/count/sync so credentials and attachments stay in the provider-account owner namespace while user_id remains the listing actor - Use deployment_row.user_id for shared delete cleanup; confirmed zero rows → 404; unconfirmed DELETE rowcount → 500 - Keep shared update rollback in the deployment owner namespace - Add apply_owned_or_visible_prefilter so scoped API keys with a concrete visible-id set do not auto-include owned deployments (owner override still unions via restrict_to_owned_or_visible) - Document None vs [] at the deployment list prefilter call site - Enforce flow:deploy before provider mutation on create, update (added versions), and snapshot replacement via ensure_flows_permission - Authorize deployment:create before existing-resource provider get/create - Add route, CRUD, listing, and guard coverage for the above
Guard-retry was syncing with the actor user_id, so shared deleters/movers could not reconcile the owner's deployments. Resolve owners from the flow rows and sync per owner instead.
HzaRashid
force-pushed
the
fix/deployment-rbac
branch
from
July 20, 2026 22:29
5d7a842 to
f90b1fe
Compare
github-actions
Bot
added
bug
github-actions
Bot
added
bug
github-actions
Bot
added
bug
erichare
marked this pull request as ready for review
July 23, 2026 15:07
github-actions
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/backend/tests/unit/api/v1/test_deployment_sync.py (1)
2759-2817: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a real DB fixture for
flow_ids_for_version_idstests instead of mockingdb.exec.These new tests mock
db.exec/result.all()directly rather than exercising the realselect(FlowVersion.flow_id)...query against a DB session, unlike sibling authorization-prefilter tests added in this same PR (e.g.test_deployment_crud_authz_prefilter.py) which use a realasync_sessionfixture with actualFlow/FlowVersionrows. A real-session variant would catch query-shape regressions (e.g. wrong column, wrong filter) that a mocked.all()return value cannot.Based on path instructions: "Warn when backend pytest files rely on excessive mocks that obscure what is actually being tested, replace mocks with real objects or test doubles when mocks become excessive, and prefer integration tests when unit tests are overly mocked."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/tests/unit/api/v1/test_deployment_sync.py` around lines 2759 - 2817, Replace the mocked db.exec/result.all setup in TestFlowIdsForVersionIds with a real async_session fixture and persisted Flow/FlowVersion rows. Exercise flow_ids_for_version_ids against the actual query, preserving coverage for empty input, partial matches raising HTTP 404, and deduplication when multiple versions share a flow.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/base/langflow/services/authorization/guards.py`:
- Around line 522-558: Validate that the results returned by batch_enforce match
len(flow_ids) before calling _audit_flow_decision_batch or zip(...,
strict=True). On a mismatch, follow the existing fail-closed behavior: audit a
deny decision with the relevant domain and error details, then raise the
documented 403 HTTPException instead of allowing ValueError to escape.
In `@src/backend/base/langflow/services/database/models/deployment/crud.py`:
- Line 627: Update the sync delete path and its widened batch helpers around
delete_deployments_by_ids() to explicitly handle UNCONFIRMED_DELETE_ROWCOUNT.
Ensure non-strict callers propagate the sentinel, log the undetermined outcome,
or return the established structured result instead of treating the deletion as
completed; preserve existing behavior for confirmed rowcounts.
In
`@src/backend/tests/unit/services/database/test_deployment_crud_authz_prefilter.py`:
- Around line 106-109: Remove the redundant pytest.mark.asyncio decorator from
test_list_deployments_page_allowed_ids_skips_owner_when_override_disabled in
src/backend/tests/unit/services/database/test_deployment_crud_authz_prefilter.py
and test_delete_by_id_negative_rowcount_returns_unconfirmed in
src/backend/base/langflow/tests/services/database/models/deployment/test_crud.py;
leave both async test functions unchanged because asyncio_mode = 'auto' detects
them automatically.
---
Nitpick comments:
In `@src/backend/tests/unit/api/v1/test_deployment_sync.py`:
- Around line 2759-2817: Replace the mocked db.exec/result.all setup in
TestFlowIdsForVersionIds with a real async_session fixture and persisted
Flow/FlowVersion rows. Exercise flow_ids_for_version_ids against the actual
query, preserving coverage for empty input, partial matches raising HTTP 404,
and deduplication when multiple versions share a flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6471a6a3-16eb-4aca-ad32-e0083fbb2b04
📒 Files selected for processing (22)
src/backend/base/langflow/api/v1/deployments.pysrc/backend/base/langflow/api/v1/flows.pysrc/backend/base/langflow/api/v1/mappers/deployments/helpers.pysrc/backend/base/langflow/api/v1/mappers/deployments/sync.pysrc/backend/base/langflow/api/v1/projects.pysrc/backend/base/langflow/services/authorization/__init__.pysrc/backend/base/langflow/services/authorization/guards.pysrc/backend/base/langflow/services/authorization/listing.pysrc/backend/base/langflow/services/authorization/utils.pysrc/backend/base/langflow/services/database/models/deployment/crud.pysrc/backend/base/langflow/tests/services/database/models/deployment/test_crud.pysrc/backend/base/langflow/tests/services/database/models/deployment/test_in_memory.pysrc/backend/base/langflow/tests/services/database/models/flow_version/test_in_memory.pysrc/backend/tests/unit/api/v1/test_deployment_guard_retry.pysrc/backend/tests/unit/api/v1/test_deployment_route_handlers.pysrc/backend/tests/unit/api/v1/test_deployment_sync.pysrc/backend/tests/unit/api/v1/test_deployments_telemetry.pysrc/backend/tests/unit/services/authorization/test_guards.pysrc/backend/tests/unit/services/authorization/test_visible_id_prefilter.pysrc/backend/tests/unit/services/database/test_deployment_crud_authz_prefilter.pysrc/backend/tests/unit/services/database/test_deployment_crud_filters.pysrc/backend/tests/unit/services/database/test_deployment_crud_project_filter.py
💤 Files with no reviewable changes (1)
- src/backend/base/langflow/api/v1/projects.py
github-actions
Bot
added
bug
Member
|
Addressed the remaining CodeRabbit review-summary nitpick in e074f82: TestFlowIdsForVersionIds now uses a real AsyncSession with persisted User, Flow, and FlowVersion rows for empty-input, partial-match/404, and shared-flow deduplication coverage. Also addressed all three inline findings and resolved their threads:
Additionally fixed the external-access ceiling ordering so flow:deploy remains admin-only when AUTHZ_ENABLED is false. Merge conflicts with release-1.12.0 are resolved. Validation: Ruff clean; 172 affected backend tests passed. |
Contributor
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-1.12.0 #14104 +/- ##
==================================================
- Coverage 61.47% 61.21% -0.27%
==================================================
Files 2339 2345 +6
Lines 237145 238012 +867
Branches 35410 35527 +117
==================================================
- Hits 145787 145690 -97
- Misses 89561 90512 +951
- Partials 1797 1810 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
erichare
approved these changes
Jul 23, 2026
erichare
left a comment
erichare
left a comment
Member
There was a problem hiding this comment.
Pushed some small fixes and rebased. LGTM!
erichare
enabled auto-merge
July 23, 2026 16:09
github-actions
Bot
added
bug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit