test(authz): RBAC enforcement + share-lifecycle integration tests via a test-double enforcer#13549
Open
erichare wants to merge 1 commit into
Open
test(authz): RBAC enforcement + share-lifecycle integration tests via a test-double enforcer#13549erichare wants to merge 1 commit into
erichare wants to merge 1 commit into
Conversation
… a test-double enforcer
No automated test exercised end-to-end authorization enforcement. The OSS
authorization service is a pass-through (enforce() always returns True,
supports_cross_user_fetch() is False), so allow/deny semantics could not be
asserted against it — only guard-wiring unit tests existed.
Add a lightweight, OSS-only test-double enforcer plus integration tests that
drive the real flow routes over HTTP under a genuine allow/deny signal — no EE
Casbin package required.
- _policy_double.py: PolicyTestAuthorizationService derives allow/deny from the
seeded authz_role / authz_role_assignment / authz_share rows and sets
SUPPORTS_CROSS_USER_FETCH=True. install_policy_authz() swaps it onto the
service manager (str-enum keys make ServiceType interchangeable) and flips
AUTHZ_ENABLED=true / AUTHZ_SUPERUSER_BYPASS=false, restoring both on exit, so
every get_authorization_service() call site (guards, fetch, listing, helpers)
sees it for the duration of a test. Ships seed helpers (roles/assignments/
shares) so tests stay declarative.
- test_rbac_enforcement_integration.py:
* Role matrix (Phase 1.11) on flow routes, with flows owned by a separate user
so the guards' owner-override does not mask the role decision:
- viewer: read + execute (build) allowed; write/delete -> 404; create -> 403
- developer: write + create allowed; delete -> 404
- admin: full, including delete
* Share lifecycle (Phase 3.13): Alice shares a flow with Bob ->
Bob GET/PATCH/build succeed; without the share every fetch route -> 404
(UUID-privacy mask). A read-level share grants GET but 404s PATCH, proving
permission_level (not mere share presence) is enforced.
* Domain resolution: a workspace-scoped grant authorizes a flow in that
workspace but 404s a flow in another — trips if _resolve_authz_domain
regresses.
In each test a cross-user GET -> 200 (only possible with enforcement on +
share-aware fetch) is the linchpin proving the paired deny is real enforcement,
not an owner-scoped fetch miss; removing a guard flips a 404 to 200/200.
Closes the enforcement integration-test gap (D) for OSS RBAC on release-1.10.0.
Per the ticket's triage note, the reusable test-double also stands alone if the
team prefers the deny-path matrix to live in EE.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughAdds a test-mode authorization enforcer ( ChangesRBAC test enforcement infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 6 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
Contributor
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
![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
🤖 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/services/authorization/_policy_double.py`:
- Around line 226-239: seed_system_roles currently leaves preexisting AuthzRole
rows unchanged, which causes non-deterministic policies; update the function
(seed_system_roles) to, for each found existing AuthzRole (by AuthzRole.name),
overwrite its permissions, is_system flag, and description to match
SYSTEM_ROLE_PERMISSIONS and the test seed description (convert permissions to a
list), flush after changes, and then commit — ensuring returned ids map to the
updated/created role IDs so the test RBAC matrix is deterministic.
- Around line 200-209: The helper _assignment_covers currently treats
assignments with domain_id is None as global; change it so only assignments with
domain_type == "global" are treated as global (i.e., return False when domain_id
is None for non-global types) and add input validation in the role assignment
path (e.g., the assign_role function) to raise a ValueError when domain_type !=
"global" and domain_id is None so malformed scoped grants fail fast; update
logic around _assignment_covers and assign_role to enforce that only explicit
global domain_type grants global coverage.
In
`@src/backend/tests/unit/services/authorization/test_rbac_enforcement_integration.py`:
- Around line 206-229: The test_read_only_share_allows_get_but_denies_write test
only asserts PATCH is denied; add an additional request that exercises the
build/execute endpoint and assert it is also denied (404) to ensure
permission_level="read" doesn't grant build/execute. Inside the same test (after
the existing PATCH check) use the test client to call the build/execute API for
the flow_id (the same resource created above) with bob_headers and expect a 404
status; this complements the existing client.get and client.patch assertions and
uses the same setup (create_user_share with permission_level="read") so the
negative case for build/execute is covered.
🪄 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: c11f0885-228a-44af-8bb5-c0092939f819
📒 Files selected for processing (2)
src/backend/tests/unit/services/authorization/_policy_double.pysrc/backend/tests/unit/services/authorization/test_rbac_enforcement_integration.py
Comment on lines
+200
to
+209
| def _assignment_covers(assignment: AuthzRoleAssignment, request_domain: str) -> bool: | ||
| """A global assignment covers every domain; a scoped one must match exactly. | ||
|
|
||
| The scoped form is intentionally exact (``{domain_type}:{domain_id}`` == | ||
| request domain) so a regression that changes the domain the route resolves | ||
| flips the decision and trips the test. | ||
| """ | ||
| if assignment.domain_type == "global" or assignment.domain_id is None: | ||
| return True | ||
| return f"{assignment.domain_type}:{assignment.domain_id}" == request_domain |
Contributor
There was a problem hiding this comment.
Don't widen malformed scoped grants into global access.
Line 207 treats any assignment with domain_id is None as covering every domain. A test that accidentally calls assign_role(..., domain_type="workspace") without a domain_id will therefore pass with global privileges and can hide domain-resolution regressions. Non-global assignments should fail fast, or at minimum return False here.
Suggested fix
def _assignment_covers(assignment: AuthzRoleAssignment, request_domain: str) -> bool:
"""A global assignment covers every domain; a scoped one must match exactly.
@@
- if assignment.domain_type == "global" or assignment.domain_id is None:
+ if assignment.domain_type == "global":
return True
+ if assignment.domain_id is None:
+ return False
return f"{assignment.domain_type}:{assignment.domain_id}" == request_domainasync def assign_role(...):
if domain_type != "global" and domain_id is None:
raise ValueError("domain_id is required for non-global assignments")🤖 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/authorization/_policy_double.py` around lines
200 - 209, The helper _assignment_covers currently treats assignments with
domain_id is None as global; change it so only assignments with domain_type ==
"global" are treated as global (i.e., return False when domain_id is None for
non-global types) and add input validation in the role assignment path (e.g.,
the assign_role function) to raise a ValueError when domain_type != "global" and
domain_id is None so malformed scoped grants fail fast; update logic around
_assignment_covers and assign_role to enforce that only explicit global
domain_type grants global coverage.
Comment on lines
+226
to
+239
| async def seed_system_roles(session: AsyncSession) -> dict[str, UUID]: | ||
| """Get-or-create viewer/developer/admin roles; return ``{name: role_id}``.""" | ||
| ids: dict[str, UUID] = {} | ||
| for name, permissions in SYSTEM_ROLE_PERMISSIONS.items(): | ||
| existing = (await session.exec(select(AuthzRole).where(AuthzRole.name == name))).first() | ||
| if existing is None: | ||
| existing = AuthzRole( | ||
| name=name, description=f"{name} (test seed)", is_system=True, permissions=list(permissions) | ||
| ) | ||
| session.add(existing) | ||
| await session.flush() | ||
| ids[name] = existing.id | ||
| await session.commit() | ||
| return ids |
Contributor
There was a problem hiding this comment.
Re-seed existing roles to keep the policy matrix deterministic.
Line 230 only keys on AuthzRole.name and leaves preexisting permissions untouched. If the test DB already contains viewer / developer / admin rows from an older migration or another fixture, this helper silently inherits that policy instead of SYSTEM_ROLE_PERMISSIONS, so these RBAC assertions stop being self-contained.
Suggested fix
async def seed_system_roles(session: AsyncSession) -> dict[str, UUID]:
"""Get-or-create viewer/developer/admin roles; return ``{name: role_id}``."""
ids: dict[str, UUID] = {}
for name, permissions in SYSTEM_ROLE_PERMISSIONS.items():
existing = (await session.exec(select(AuthzRole).where(AuthzRole.name == name))).first()
if existing is None:
existing = AuthzRole(
name=name, description=f"{name} (test seed)", is_system=True, permissions=list(permissions)
)
session.add(existing)
await session.flush()
+ else:
+ existing.permissions = list(permissions)
+ existing.is_system = True
+ existing.description = f"{name} (test seed)"
ids[name] = existing.id
await session.commit()
return ids📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async def seed_system_roles(session: AsyncSession) -> dict[str, UUID]: | |
| """Get-or-create viewer/developer/admin roles; return ``{name: role_id}``.""" | |
| ids: dict[str, UUID] = {} | |
| for name, permissions in SYSTEM_ROLE_PERMISSIONS.items(): | |
| existing = (await session.exec(select(AuthzRole).where(AuthzRole.name == name))).first() | |
| if existing is None: | |
| existing = AuthzRole( | |
| name=name, description=f"{name} (test seed)", is_system=True, permissions=list(permissions) | |
| ) | |
| session.add(existing) | |
| await session.flush() | |
| ids[name] = existing.id | |
| await session.commit() | |
| return ids | |
| async def seed_system_roles(session: AsyncSession) -> dict[str, UUID]: | |
| """Get-or-create viewer/developer/admin roles; return ``{name: role_id}``.""" | |
| ids: dict[str, UUID] = {} | |
| for name, permissions in SYSTEM_ROLE_PERMISSIONS.items(): | |
| existing = (await session.exec(select(AuthzRole).where(AuthzRole.name == name))).first() | |
| if existing is None: | |
| existing = AuthzRole( | |
| name=name, description=f"{name} (test seed)", is_system=True, permissions=list(permissions) | |
| ) | |
| session.add(existing) | |
| await session.flush() | |
| else: | |
| existing.permissions = list(permissions) | |
| existing.is_system = True | |
| existing.description = f"{name} (test seed)" | |
| ids[name] = existing.id | |
| await session.commit() | |
| return ids |
🤖 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/authorization/_policy_double.py` around lines
226 - 239, seed_system_roles currently leaves preexisting AuthzRole rows
unchanged, which causes non-deterministic policies; update the function
(seed_system_roles) to, for each found existing AuthzRole (by AuthzRole.name),
overwrite its permissions, is_system flag, and description to match
SYSTEM_ROLE_PERMISSIONS and the test seed description (convert permissions to a
list), flush after changes, and then commit — ensuring returned ids map to the
updated/created role IDs so the test RBAC matrix is deterministic.
Comment on lines
+206
to
+229
| async def test_read_only_share_allows_get_but_denies_write(client): | ||
| """A read-level share grants GET but not PATCH — permission_level is enforced, not mere presence.""" | ||
| settings = get_settings_service() | ||
| alice_id = await _make_user(f"alice_{uuid4().hex}") | ||
| bob_username = f"bob_{uuid4().hex}" | ||
| bob_id = await _make_user(bob_username) | ||
| flow_id = await _make_flow(alice_id, f"aliceflow_{uuid4().hex}") | ||
| bob_headers = await _login(client, bob_username) | ||
|
|
||
| async with session_scope() as session: | ||
| await create_user_share( | ||
| session, | ||
| resource_type="flow", | ||
| resource_id=flow_id, | ||
| target_user_id=bob_id, | ||
| permission_level="read", | ||
| created_by=alice_id, | ||
| ) | ||
|
|
||
| with install_policy_authz(settings): | ||
| assert (await client.get(f"api/v1/flows/{flow_id}", headers=bob_headers)).status_code == 200 | ||
| # write is not granted by a read-level share -> deny -> 404 | ||
| patch = await client.patch(f"api/v1/flows/{flow_id}", headers=bob_headers, json={"name": "nope"}) | ||
| assert patch.status_code == 404 |
Contributor
There was a problem hiding this comment.
Assert that read-only shares also deny build/execute.
This case only proves that a read share blocks PATCH. Because execute is modeled independently from write in the new test double, a regression that accidentally grants build access to permission_level="read" would still pass here. Add a build request and expect the same 404 mask.
Suggested fix
with install_policy_authz(settings):
assert (await client.get(f"api/v1/flows/{flow_id}", headers=bob_headers)).status_code == 200
# write is not granted by a read-level share -> deny -> 404
patch = await client.patch(f"api/v1/flows/{flow_id}", headers=bob_headers, json={"name": "nope"})
assert patch.status_code == 404
+ build = await client.post(f"api/v1/build/{flow_id}/flow", headers=bob_headers, json={})
+ assert build.status_code == 404As per coding guidelines, "For API endpoints, verify both success and error response testing" and "Ensure tests cover both positive and negative scenarios where appropriate."
🤖 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/authorization/test_rbac_enforcement_integration.py`
around lines 206 - 229, The test_read_only_share_allows_get_but_denies_write
test only asserts PATCH is denied; add an additional request that exercises the
build/execute endpoint and assert it is also denied (404) to ensure
permission_level="read" doesn't grant build/execute. Inside the same test (after
the existing PATCH check) use the test client to call the build/execute API for
the flow_id (the same resource created above) with bob_headers and expect a 404
status; this complements the existing client.get and client.patch assertions and
uses the same setup (create_user_share with permission_level="read") so the
negative case for build/execute is covered.
Source: Coding guidelines
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
1 participant
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.
Problem
No automated test exercised end-to-end authorization enforcement. The OSS authorization service is a pass-through —
enforce()always returnsTrueandsupports_cross_user_fetch()isFalse— so allow/deny semantics could not be asserted against it. Only guard-wiring unit tests existed; nothing proved that the route guards, domain resolution, share-aware fetch, anddeny_to_404masking actually deny the right requests. (Closes gap D of the OSS RBAC rollout onrelease-1.10.0.)Approach — an OSS-only test-double enforcer
The blocker is that you need a real allow/deny signal without the EE Casbin package. This PR adds a lightweight in-test enforcer that reads the same policy tables a real plugin would:
_policy_double.pyPolicyTestAuthorizationService(BaseAuthorizationService)— derives allow/deny from seededauthz_role/authz_role_assignment/authz_sharerows, honorsAUTHZ_SUPERUSER_BYPASS, and setsSUPPORTS_CROSS_USER_FETCH=True.install_policy_authz(settings)— a context manager that swaps the service registered on the service manager (str-enum keys makeServiceType.AUTHORIZATION_SERVICEinterchangeable across the lfx/langflow enums) so everyget_authorization_service()call site (guards, fetch, listing, helpers) resolves to the double, flipsAUTHZ_ENABLED=true/AUTHZ_SUPERUSER_BYPASS=false, and restores all of it on exit.seed_system_roles(get-or-create, so it works whether or not the test DB ran the migration seed),assign_role,create_user_share.Installing via the service manager (rather than per-module monkeypatching) means the tests run through the real HTTP stack and exercise the production wiring end to end.
Tests —
test_rbac_enforcement_integration.pyRole matrix (Phase 1.11) on real flow routes. Flows are owned by a separate user so the guards' owner-override doesn't mask the role decision — these assert the role, not ownership:
Share lifecycle (Phase 3.13): Alice shares a flow with Bob → Bob
GET/PATCH/buildall succeed; without the share every fetch route returns 404 (UUID-privacy mask, not 403). A separate test gives Bob a read-level share and assertsGET200 butPATCH404 — provingpermission_levelis enforced, not mere share presence.Domain resolution: a
workspace-scoped grant authorizes a flow in that workspace (200) but 404s a flow in a different workspace — this trips if_resolve_authz_domainregresses (the route would resolve the wrong domain and the scoped grant would stop matching).Why these assertions are meaningful (acceptance criteria)
GET → 200is the linchpin: it's only reachable with enforcement on and share-aware fetch widening the query. The pairedPATCH/DELETE → 404is therefore a genuine deny, not an owner-scoped fetch miss. Remove the write/delete guard and that 404 flips to 200 → test fails.== 404fails if it were 403).Testing
Backend
ruffclean; all pre-commit hooks (incl. detect-secrets) pass. No production code changed — additive test-only.Triage note
This implements the full version (test-double + role matrix + share lifecycle + domain resolution). Per the ticket's triage note, if the team prefers the deny-path matrix to live in the EE repo,
_policy_double.pystands alone as "the test-double enforcer fixture so OSS can self-test enforcement semantics" and the matrix tests can move without touching it.Summary by CodeRabbit