fix(authz): support scoped project visibility - #14429
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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe change adds scoped visibility rules and provider authorization checks. Project listings now include ownership metadata. Frontend project labels, default selection, and rename behavior use project IDs, owner-qualified names, and write permissions. ChangesVisibility and project ownership
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant deployments.py
participant has_visible_deployment_for_provider
participant DeploymentDatabase
Client->>deployments.py: request deployment listing
deployments.py->>has_visible_deployment_for_provider: verify provider visibility
has_visible_deployment_for_provider->>DeploymentDatabase: apply provider and visibility scope
DeploymentDatabase-->>has_visible_deployment_for_provider: visibility result
has_visible_deployment_for_provider-->>deployments.py: visible or not visible
deployments.py->>DeploymentDatabase: resolve provider and authorize READ
DeploymentDatabase-->>deployments.py: deployments or authorization failure
deployments.py-->>Client: deployment list or 404
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
✅ Migration Validation Passed All migrations follow the Expand-Contract pattern correctly. |
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
github-actions
Bot
added
bug
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-1.12.0 #14429 +/- ##
==================================================
+ Coverage 61.95% 63.40% +1.44%
==================================================
Files 2417 2391 -26
Lines 242524 243318 +794
Branches 36184 37370 +1186
==================================================
+ Hits 150251 154268 +4017
+ Misses 90354 87125 -3229
- Partials 1919 1925 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/backend/tests/unit/api/v1/test_deployment_route_handlers.py (1)
875-882: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the configured async test mode.
Remove these explicit async runner markers.
asyncio_mode = "auto"already runs theseasync deftests. If the AnyIO marker is required for a distinct backend policy, document that policy and configure it explicitly.
src/backend/tests/unit/api/v1/test_deployment_route_handlers.py#L875-L882: Remove@pytest.mark.asyncio.src/backend/tests/unit/api/v1/test_deployment_route_handlers.py#L915-L920: Remove@pytest.mark.asyncio.src/backend/tests/unit/api/v1/test_deployment_route_handlers.py#L953-L961: Remove@pytest.mark.asyncio.src/backend/tests/unit/services/authorization/test_visibility_scope_prefilter.py#L150-L151: Remove@pytest.mark.anyiounless the test requires a documented alternate backend.src/backend/tests/unit/services/database/test_deployment_crud_authz_prefilter.py#L200-L201: Remove@pytest.mark.asyncio.Based on learnings: pytest-asyncio uses
asyncio_mode = 'auto'; tests should avoid unnecessary async markers.🤖 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_route_handlers.py` around lines 875 - 882, Remove the unnecessary async test markers under configured automatic async mode: delete `@pytest.mark.asyncio` from the three tests in src/backend/tests/unit/api/v1/test_deployment_route_handlers.py at lines 875-882, 915-920, and 953-961; delete `@pytest.mark.anyio` from the test in src/backend/tests/unit/services/authorization/test_visibility_scope_prefilter.py at lines 150-151 unless it explicitly requires a documented alternate backend; and delete `@pytest.mark.asyncio` from the test in src/backend/tests/unit/services/database/test_deployment_crud_authz_prefilter.py at lines 200-201. Preserve the async test bodies and configure/document an alternate backend only if required.Source: Learnings
src/frontend/src/pages/MainPage/pages/homePage/__tests__/project-owner-label.test.tsx (1)
37-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd an integration-level owner-label test.
This setup mocks the header and MCP tab that the assertions inspect. The test verifies values passed to mocks, not the rendered owner-label behavior. Keep focused unit tests, but add a provider-backed test that renders the real project-name consumers.
As per coding guidelines, frontend tests must avoid excessive mocks and verify meaningful behavior.
🤖 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/frontend/src/pages/MainPage/pages/homePage/__tests__/project-owner-label.test.tsx` around lines 37 - 97, Add an integration-level test that renders the real project-name consumers instead of mocked Header and CustomMcpServerTab components, while retaining focused unit tests as appropriate. Use the necessary providers and realistic folder/ownership data to verify the rendered owner label behavior, and remove or bypass only the mocks that prevent observing it.Source: Coding guidelines
🤖 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/tests/unit/api/v1/test_projects.py`:
- Around line 159-192: Extend
test_read_projects_qualifies_visible_same_named_projects_by_owner with a visible
project whose user_id is None, include it in the mocked project results, and
assert the returned project has owner_username set to None and is_owner set to
False while preserving the existing owner qualification assertions.
In
`@src/frontend/src/components/core/folderSidebarComponent/components/sideBarFolderButtons/index.tsx`:
- Line 456: Make both project test IDs unique by appending item.id: update the
sidebar-nav test ID in
src/frontend/src/components/core/folderSidebarComponent/components/sideBarFolderButtons/index.tsx
lines 456-456 and the more-options-button test ID in
src/frontend/src/components/core/folderSidebarComponent/components/sideBarFolderButtons/components/select-options.tsx
lines 60-62, preserving the existing item.name values.
In `@src/frontend/src/utils/__tests__/project-display-name.test.ts`:
- Around line 43-73: Add an empty-input edge-case test to the
getDefaultProjectId test suite, passing an empty project list and the default
project name, and assert that it returns an empty string to cover folder-store
initialization when no projects are visible.
---
Nitpick comments:
In `@src/backend/tests/unit/api/v1/test_deployment_route_handlers.py`:
- Around line 875-882: Remove the unnecessary async test markers under
configured automatic async mode: delete `@pytest.mark.asyncio` from the three
tests in src/backend/tests/unit/api/v1/test_deployment_route_handlers.py at
lines 875-882, 915-920, and 953-961; delete `@pytest.mark.anyio` from the test in
src/backend/tests/unit/services/authorization/test_visibility_scope_prefilter.py
at lines 150-151 unless it explicitly requires a documented alternate backend;
and delete `@pytest.mark.asyncio` from the test in
src/backend/tests/unit/services/database/test_deployment_crud_authz_prefilter.py
at lines 200-201. Preserve the async test bodies and configure/document an
alternate backend only if required.
In
`@src/frontend/src/pages/MainPage/pages/homePage/__tests__/project-owner-label.test.tsx`:
- Around line 37-97: Add an integration-level test that renders the real
project-name consumers instead of mocked Header and CustomMcpServerTab
components, while retaining focused unit tests as appropriate. Use the necessary
providers and realistic folder/ownership data to verify the rendered owner label
behavior, and remove or bypass only the mocks that prevent observing it.
🪄 Autofix
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: d4871e2b-6054-44c9-8bca-2bc352527acf
📒 Files selected for processing (24)
.secrets.baselinesrc/backend/base/langflow/api/v1/deployments.pysrc/backend/base/langflow/api/v1/projects.pysrc/backend/base/langflow/services/authorization/listing.pysrc/backend/base/langflow/services/database/models/deployment/crud.pysrc/backend/base/langflow/services/database/models/folder/model.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_guards.pysrc/backend/tests/unit/services/authorization/test_visibility_scope_prefilter.pysrc/backend/tests/unit/services/database/test_deployment_crud_authz_prefilter.pysrc/frontend/src/components/core/appHeaderComponent/components/FlowMenu/__tests__/FlowMenu.spec.tsxsrc/frontend/src/components/core/appHeaderComponent/components/FlowMenu/index.tsxsrc/frontend/src/components/core/folderSidebarComponent/components/sideBarFolderButtons/__tests__/project-create-error.test.tsxsrc/frontend/src/components/core/folderSidebarComponent/components/sideBarFolderButtons/components/input-edit-folder-name.tsxsrc/frontend/src/components/core/folderSidebarComponent/components/sideBarFolderButtons/components/select-options.tsxsrc/frontend/src/components/core/folderSidebarComponent/components/sideBarFolderButtons/index.tsxsrc/frontend/src/controllers/API/queries/folders/use-get-folders.tssrc/frontend/src/pages/MainPage/entities/index.tsxsrc/frontend/src/pages/MainPage/pages/homePage/__tests__/project-owner-label.test.tsxsrc/frontend/src/pages/MainPage/pages/homePage/index.tsxsrc/frontend/src/utils/__tests__/project-display-name.test.tssrc/frontend/src/utils/project-display-name.tssrc/lfx/src/lfx/services/authorization/base.py
github-actions
Bot
added
bug
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
keval718
left a comment
keval718
left a comment
There was a problem hiding this comment.
The security narrowing (deployment provider gate) and the rename-by-id fix are correct and well tested. Two things block: a Postgres-only 500 in the exact path this PR exists to enable, and ~6 Playwright specs silently broken by the testid change.
Blocking
1. literal(None) IS NULL will 500 on Postgres — in the primary new code path
src/backend/base/langflow/api/v1/projects.py:384 passes workspace_expression=literal(project.workspace_id). The new unassigned-workspace branch at src/backend/base/langflow/services/authorization/listing.py:187 then calls resolved_workspace.is_(None) on that bind param. Compiled against the asyncpg dialect:
workspace_id=None -> $1 IS NULL # no cast
workspace_id=<uuid> -> $1::UUID IS NULL # cast inferred from the value
Postgres rejects an untyped parameter in an IS NULL predicate: 42P18 could not determine data type of parameter $1. The pre-existing resolved_workspace.in_(visibility.workspace_ids) survives because PG infers the type from the IN list — IS NULL has nothing to infer from.
Failure scenario: a plugin returns include_unassigned_workspace=True, a user opens a shared project whose workspace_id is NULL with ?page=1&size=12, and GET /api/v1/projects/{id} 500s. Every backend test here runs on SQLite, where ? IS NULL is legal — so CI stays green.
Suggested fix, short-circuiting in Python instead of emitting a bind param:
workspace_expression=sa.null() if project.workspace_id is None else literal(project.workspace_id),or have restrict_to_owned_or_visible_scope take a workspace_is_null: bool | None and emit true()/false(). Worth a Postgres-marked regression test — SQLite parity tests structurally cannot catch this class of bug.
2. Project sidebar testid change breaks Playwright suites, none updated
src/frontend/src/components/core/folderSidebarComponent/components/sideBarFolderButtons/index.tsx:456 (sidebar-nav-${name} -> sidebar-nav-${name}-${id}) and .../components/select-options.tsx:60 (more-options-button_${name} -> ..._${name}_${id}). Playwright's getByTestId is an exact match, so these break:
src/frontend/tests/core/features/folders.spec.ts:56,83,98,111,171,187,192,199src/frontend/tests/core/features/folder-deletion-integrity.spec.ts(27 refs)src/frontend/tests/extended/features/mcp-server-starter-projects.spec.ts:65,88,92src/frontend/tests/extended/regression/general-bugs-move-flow-from-folder.spec.ts:51,59,71,83src/frontend/tests/utils/clean-old-folders.ts:18— shared helper, fans out further
Separately: the id suffix alone achieves uniqueness. Keeping the raw, unslugified item.name (sidebar-nav-Starter Project-<uuid>) makes selectors doubly brittle and is inconsistent with more-options-button, which does run convertTestName. Prefer data-testid={sidebar-nav-${item.id}} and have e2e resolve name -> id, or at minimum slugify.
Should fix
3. Null-owner projects render a raw UUID to OSS users
src/frontend/src/utils/project-display-name.ts:12 — ${name} — ${owner_username ?? id ?? "unknown owner"}. read_projects deliberately surfaces user_id IS NULL projects on the OSS path (projects.py:257), and the new test asserts owner_username=None, is_owner=False for exactly those rows. On a default OSS install with a legacy null-owner folder the sidebar renders My Projects — 3f2a19c8-...: a UUID leaked into the UI, and a project nobody owns mislabelled as foreign.
if (project.is_owner !== false || !project.owner_username) return project.name;
return t("project.ownedBy", { name: project.name, owner: project.owner_username });That also removes the unreachable "unknown owner" literal.
4. Owner qualifier is untranslated
Same file — the — separator and the whole format are hardcoded English/typography, while adjacent code uses t("folder.optionsFor", { name }). AGENTS.md conventions want user-facing copy behind i18n; a translation key also lets RTL locales and CJK punctuation render correctly.
5. is_owner handled inconsistently across the two new helpers
getProjectDisplayName treats is_owner !== false as owned, so undefined is safe. getDefaultProjectId (project-display-name.ts:21) uses truthy project.is_owner, so undefined skips both find calls and falls through to projects[0]. The old code matched on name === defaultFolderName with no ownership dependency.
Failure scenario: any client hitting a backend that doesn't emit is_owner (version skew, /customization overrides) silently loses default-project selection by name. Add the name match as a final fallback before projects[0], or normalize is_owner once at the query boundary.
6. getDefaultProjectId returns "" where the old code returned undefined
use-get-folders.ts:26 now always stores a string. src/frontend/src/pages/MainPage/pages/deploymentsPage/components/deployment-stepper-modal.tsx:76 reads currentFlowProjectId ?? folderId ?? myCollectionId ?? undefined — with "" the ?? undefined guard no longer fires and an empty project_id can reach the API. Returning undefined and leaving the existing ?? "" call sites to cope keeps the old contract.
Confirmed good
- No OSS widening.
include_unassigned_workspaceandexcluded_workspace_project_idsdefault to false/empty, OSSvisible_scope_prefilterstill returnsNone, andread_projectskeeps its owner-scoped query. Thehas_cross_user_accessaddition only fires on the new flag. Matches the AGENTS.md rule that enablingLANGFLOW_AUTHZ_ENABLEDwithout a plugin cannot widen visibility. - Deployment provider gate strictly narrows.
deployments.py:793-830now requires provider-specific visible evidence viahas_visible_deployment_for_providerbefore the unscoped lookup, then masks the subsequent 403 withdeny_to_404— consistent with the 11 otherdeny_to_404sites in the same file. The evidence query reuses_scope_to_owner_or_allowed, so it cannot drift from the page/count predicate. Good catch on the existence oracle. - SQL / in-memory parity between
restrict_to_owned_or_visible_scopeandresource_visible_in_scopeis correct for both the exclusion gate and the unassigned branch, andtest_workspace_scope_sql_matches_in_memory_for_project_nulls_and_exclusionsexercises it against a real session rather than string-matching SQL. - Rename targeting by id is the right fix, and
project-create-error.test.tsx:222proves both halves — foreign project not editable, own project renames withfolderId: "own-id". - Frontend test claim verified. I ran the four suites locally: 19 passed, 4 suites, matching the PR description.
Nitpicks
projects.py:290—FolderListRead(**FolderRead.model_validate(...).model_dump(), ...)validates twice.FolderListRead.model_validate(project, from_attributes=True)with the two extras assigned afterwards is cheaper and won't silently break ifFolderReadever gains a colliding field name.sideBarFolderButtons/index.tsx:66—ProjectRenamePermissionduplicates thecan(id, "write")computationselect-options.tsx:34already does. A shareduseProjectCanWrite(id)hook would keep the two from drifting.sideBarFolderButtons/index.tsx:496— theonClick={(e) => e.stopPropagation()}on the SelectOptions wrapper was dropped without mention in the PR body. It looks safe (no ancestor click handler), but it's an unexplained behavior change riding along in an authz PR.- Agreed with CodeRabbit on dropping the redundant
@pytest.mark.asyncio/@pytest.mark.anyiomarkers givenasyncio_mode = "auto". Disagreed on its request for a provider-backed integration test inproject-owner-label.test.tsx— the unit coverage inproject-display-name.test.tsplus the sidebar integration test is proportionate. - PR body has no
Fixes #Nreference; AGENTS.md asks for the issue link.
|
Thanks @keval718 — addressed in 954642f. The null-workspace prefilter now emits SQL NULL with an asyncpg compilation regression; project row/options selectors are UUID-only and the affected specs/helpers resolve names to IDs; and owner labels/default selection now handle i18n, null owners, version skew, and undefined. I also removed the redundant async markers and double Folder validation. Focused backend, Jest, and affected Playwright scenarios are green. |
github-actions
Bot
added
bug
github-actions
Bot
added
bug
|
Thanks @keval718 — the upgrade/scoping follow-up is pushed. Existing Global Viewer/Developer/Admin assignments now reconcile to the documented flow permissions; roleless users remain owner/direct-share only; Default workspace covers only null-workspace projects; and ownerless Starter Project flows/deployments stay excluded from automatic scope access while exact shares remain additive. The exact-pin Enterprise suite is green (654 passed, 9 skipped). |
2 participants
Summary
workspace_idis null, with SQL/in-memory parity and reserved-project exclusions.Validation
135 passed, 1 skippedacross project, authorization visibility/guard, and deployment prefilter/route regressions.19 passedacross the four focused frontend suites.git diff --checkpass.Summary by CodeRabbit