fix: complete shared resource access (LE-1828) - #14101
Conversation
|
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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughIntroduces structured resource visibility scopes, replaces ID-based authorization prefilters, and propagates scoped authorization through resource listings, deployment queries, Memory Base operations, variables, files, project downloads, and related tests. ChangesStructured visibility authorization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant API route
participant AuthorizationService
participant ScopeQuery
participant Database
API route->>AuthorizationService: request resource visibility
AuthorizationService-->>API route: return ResourceVisibilityScope
API route->>ScopeQuery: apply owner and visibility predicates
ScopeQuery->>Database: execute scoped query
Database-->>API route: return authorized resources
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 7 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (7 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 |
229202c to
5a5d99e
Compare
5a5d99e to
459b20b
Compare
459b20b to
0fd9d14
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/base/langflow/api/v1/mappers/deployments/helpers.py (1)
889-922: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftUse the deployment owner’s namespace during shared-list synchronization.
visibility_scopecan now return foreign-owned deployments, but provider lookup and attachment synchronization still use the requestinguser_id. On owner-namespaced providers, this can report valid owner resources as missing and trigger the stale-row deletion at Line 951.Thread the provider/resource owner separately—or group rows by owner—and use it for provider lookup, attachment cleanup, and recounting. Add a regression test where actor and deployment owner differ.
🤖 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/base/langflow/api/v1/mappers/deployments/helpers.py` around lines 889 - 922, Update the shared deployment synchronization flow around list_deployments_page so provider operations use each deployment’s owner namespace rather than the requesting user_id. Group rows by owner or carry the owner through provider lookup, attachment cleanup, stale-row deletion, and recounting, while preserving actor-based authorization via allowed_ids. Add a regression test covering an actor listing a deployment owned by a different user.Source: Coding guidelines
🧹 Nitpick comments (1)
src/backend/tests/unit/services/database/test_deployment_crud_authz_prefilter.py (1)
167-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the redundant asyncio markers.
The repository uses pytest-asyncio auto mode, so these decorators add unnecessary per-test configuration.
src/backend/tests/unit/services/database/test_deployment_crud_authz_prefilter.py#L167-L167: remove@pytest.mark.asyncio.src/backend/tests/unit/test_memory_bases.py#L560-L560: remove@pytest.mark.asyncio.src/backend/tests/unit/api/v1/test_authz_share_routes.py#L154-L154: remove@pytest.mark.asyncio.Based on learnings, async tests under
tests/should rely on automatic detection.🤖 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/services/database/test_deployment_crud_authz_prefilter.py` at line 167, Remove the redundant `@pytest.mark.asyncio` decorators from src/backend/tests/unit/services/database/test_deployment_crud_authz_prefilter.py:167-167, src/backend/tests/unit/test_memory_bases.py:560-560, and src/backend/tests/unit/api/v1/test_authz_share_routes.py:154-154; rely on pytest-asyncio auto mode for async test detection.Source: Learnings
🤖 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/api/v2/files.py`:
- Around line 487-503: Update the UserFile query’s post-visibility filtering to
exclude reserved internal MCP filenames derived from each row’s owner, rather
than only the current actor’s `_mcp_servers_<actor-id>` name. Apply this
owner-based exclusion after restrict_to_owned_or_visible_scope while preserving
access to ordinary files.
In `@src/backend/base/langflow/services/variable/service.py`:
- Around line 213-242: Update the shared-variable resolution block around
get_authorization_service and list_visible_resource_ids to use
get_resource_visibility instead. Permit all_resources scopes, constrain queries
to concrete resource_ids, and fail closed for workspace/project-only scopes
because variables lack canonical domain columns. Add a runtime test covering a
scope-native visibility provider alongside the existing legacy-adapter test.
In `@src/backend/tests/unit/test_memory_bases.py`:
- Around line 598-611: Expand
test_list_statement_unions_owned_and_visible_memory_bases to cover
workspace/project-only visibility scopes and assert they remain restricted to
actor_id, plus an all_resources=True scope that omits the owner restriction.
Include positive, negative, and edge-case SQL assertions while preserving the
existing concrete-resource-ID coverage.
---
Outside diff comments:
In `@src/backend/base/langflow/api/v1/mappers/deployments/helpers.py`:
- Around line 889-922: Update the shared deployment synchronization flow around
list_deployments_page so provider operations use each deployment’s owner
namespace rather than the requesting user_id. Group rows by owner or carry the
owner through provider lookup, attachment cleanup, stale-row deletion, and
recounting, while preserving actor-based authorization via allowed_ids. Add a
regression test covering an actor listing a deployment owned by a different
user.
---
Nitpick comments:
In
`@src/backend/tests/unit/services/database/test_deployment_crud_authz_prefilter.py`:
- Line 167: Remove the redundant `@pytest.mark.asyncio` decorators from
src/backend/tests/unit/services/database/test_deployment_crud_authz_prefilter.py:167-167,
src/backend/tests/unit/test_memory_bases.py:560-560, and
src/backend/tests/unit/api/v1/test_authz_share_routes.py:154-154; rely on
pytest-asyncio auto mode for async test detection.
🪄 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
Run ID: 2eb65bcf-4c6d-48ab-857d-f95aa9ff6cd1
📒 Files selected for processing (30)
.secrets.baselinesrc/backend/base/langflow/api/utils/knowledge_base_service.pysrc/backend/base/langflow/api/v1/authz_shares.pysrc/backend/base/langflow/api/v1/deployments.pysrc/backend/base/langflow/api/v1/flows.pysrc/backend/base/langflow/api/v1/knowledge_bases.pysrc/backend/base/langflow/api/v1/mappers/deployments/helpers.pysrc/backend/base/langflow/api/v1/memories.pysrc/backend/base/langflow/api/v1/projects.pysrc/backend/base/langflow/api/v1/projects_files.pysrc/backend/base/langflow/api/v1/variable.pysrc/backend/base/langflow/api/v2/files.pysrc/backend/base/langflow/services/authorization/__init__.pysrc/backend/base/langflow/services/authorization/listing.pysrc/backend/base/langflow/services/database/models/deployment/crud.pysrc/backend/base/langflow/services/memory_base/service.pysrc/backend/base/langflow/services/variable/service.pysrc/backend/base/langflow/tests/services/database/models/deployment/test_crud.pysrc/backend/tests/unit/api/v1/test_authz_share_routes.pysrc/backend/tests/unit/api/v1/test_deployment_route_handlers.pysrc/backend/tests/unit/api/v1/test_projects.pysrc/backend/tests/unit/services/authorization/test_route_ceiling_guards.pysrc/backend/tests/unit/services/authorization/test_route_guard_regressions.pysrc/backend/tests/unit/services/authorization/test_unscoped_list_visibility.pysrc/backend/tests/unit/services/authorization/test_visibility_scope_prefilter.pysrc/backend/tests/unit/services/database/test_deployment_crud_authz_prefilter.pysrc/backend/tests/unit/services/variable/test_service.pysrc/backend/tests/unit/test_memory_bases.pysrc/lfx/src/lfx/services/authorization/__init__.pysrc/lfx/src/lfx/services/authorization/base.py
0fd9d14 to
6fa9372
Compare
15b274d to
34712bd
Compare
|
Addressed the remaining review-body items in 34712bd as well: deployment synchronization now keeps the requesting actor for DB visibility while using the provider-account owner for provider and attachment namespaces; the shared project ZIP path applies per-flow READ authorization; and the three redundant async markers were removed. Validation on the current release-1.12.0 head: 411 passed, 3 skipped; Ruff, formatting, diff checks, and detect-secrets all pass. |
Summary
knowledge_baseauthorization resource typeResourceVisibilityScopefor those lists so global wildcard and concrete-ID grants work without materializing every UUIDDependency / merge order
release-1.11.0so this stacked PR remains mergeable against its OSS release base.release-1.11.0as their GitHub base.Validation
all_resources, concreteresource_ids, and owner-only fail-closed behavior for workspace/project-only scopes on models without domain columnsFollow-up
Jira: LE-1828
Summary by CodeRabbit
New Features
Bug Fixes