Fix team member cannot edit agent#14612
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes canvas authorization by adding reusable sync/async wrappers and applying them across multiple agent REST routes, removing inline ChangesCanvas Authorization Consolidation
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as AgentAPI
participant UCS as UserCanvasService
Client->>API: HTTP request to agent endpoint
API->>UCS: authorization check (accessible / query) via decorator
UCS-->>API: allowed / denied
alt allowed
API-->>Client: handler executes (200 / SSE / stream)
else denied
API-->>Client: 403 JSON error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
dosubot
Bot
added
size:M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/apps/restful_apis/agent_api.py (2)
71-85:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd denial logging in the new authorization decorators.
Line 75 and Line 84 return auth failures without any log signal. Since this is a new centralized access-control flow, please log denied attempts (agent_id, tenant_id, endpoint/action) for auditability and incident triage.
As per coding guidelines,
**/*.py: Add logging for new flows.🤖 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 `@api/apps/restful_apis/agent_api.py` around lines 71 - 85, The two authorization decorators _require_canvas_access_sync and _require_canvas_owner_sync currently return failure responses without logging; add denial logging before each return by obtaining agent_id and tenant_id from kwargs, determine the endpoint/action via func.__name__ (and include request.path or request.method if request is available), and log a structured warning/error using a module logger (get logger with logging.getLogger(__name__)); include agent_id, tenant_id, endpoint/action and a short reason string, then proceed to return the existing get_json_result with RetCode.OPERATING_ERROR.
71-77:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
_require_canvas_access_syncasync-aware to properly handle async endpoints.The
wrapperfunction is synchronous but wraps multiple async endpoints (lines 185, 452, 581, 632, 809). When called on an async function, it returns an unawaited coroutine. Sinceadd_tenant_id_to_kwargs(the next decorator in the stack) checksinspect.iscoroutinefunction()on the sync wrapper—not the original endpoint—it fails to detect the coroutine and returns it without awaiting, leaking an unawaited coroutine object.Suggested fix
def _require_canvas_access_sync(func): + is_coro = inspect.iscoroutinefunction(func) `@wraps`(func) - def wrapper(*args, **kwargs): + async def wrapper(*args, **kwargs): if not UserCanvasService.accessible(kwargs.get('agent_id'), kwargs.get('tenant_id')): return get_json_result(data=False, message="Make sure you have permission to access the agent.", code=RetCode.OPERATING_ERROR) - return func(*args, **kwargs) + if is_coro: + return await func(*args, **kwargs) + return func(*args, **kwargs) return wrapper🤖 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 `@api/apps/restful_apis/agent_api.py` around lines 71 - 77, The decorator _require_canvas_access_sync must be made async-aware: change its inner wrapper to async def, keep `@wraps`(func), and inside check UserCanvasService.accessible(...) as before and then call the wrapped func using await if the original func is a coroutine (use inspect.iscoroutinefunction(func) or inspect.isawaitable on the result), otherwise call it synchronously; this ensures async endpoints wrapped by _require_canvas_access_sync are awaited (so add_tenant_id_to_kwargs can correctly detect coroutine functions) while preserving behavior for synchronous handlers.
🤖 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 `@api/apps/restful_apis/agent_api.py`:
- Around line 808-809: The route function agent_chat_completion is currently
decorated with `@_require_canvas_access_sync` which expects kwargs["agent_id"] but
agent_id is parsed from the request body inside agent_chat_completion, causing
valid requests to be rejected; remove the `@_require_canvas_access_sync` decorator
from agent_chat_completion and either (a) perform the access check explicitly
after you parse agent_id from the request body by calling the same access-check
routine (use the underlying check function or method the decorator wraps,
passing the parsed agent_id), or (b) change the route to accept agent_id as a
URL parameter so the decorator can access kwargs["agent_id"]; ensure the chosen
fix calls the same access logic that the decorator enforces (the symbol:
_require_canvas_access_sync) using the parsed agent_id.
---
Outside diff comments:
In `@api/apps/restful_apis/agent_api.py`:
- Around line 71-85: The two authorization decorators
_require_canvas_access_sync and _require_canvas_owner_sync currently return
failure responses without logging; add denial logging before each return by
obtaining agent_id and tenant_id from kwargs, determine the endpoint/action via
func.__name__ (and include request.path or request.method if request is
available), and log a structured warning/error using a module logger (get logger
with logging.getLogger(__name__)); include agent_id, tenant_id, endpoint/action
and a short reason string, then proceed to return the existing get_json_result
with RetCode.OPERATING_ERROR.
- Around line 71-77: The decorator _require_canvas_access_sync must be made
async-aware: change its inner wrapper to async def, keep `@wraps`(func), and
inside check UserCanvasService.accessible(...) as before and then call the
wrapped func using await if the original func is a coroutine (use
inspect.iscoroutinefunction(func) or inspect.isawaitable on the result),
otherwise call it synchronously; this ensures async endpoints wrapped by
_require_canvas_access_sync are awaited (so add_tenant_id_to_kwargs can
correctly detect coroutine functions) while preserving behavior for synchronous
handlers.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 378c4030-1868-4f2c-9ebe-546b56b9b600
📒 Files selected for processing (1)
api/apps/restful_apis/agent_api.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #14612 +/- ##
=======================================
Coverage 94.16% 94.16%
=======================================
Files 10 10
Lines 703 703
Branches 112 112
=======================================
Hits 662 662
Misses 25 25
Partials 16 16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/apps/restful_apis/agent_api.py (1)
71-95: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd logging in the new authorization wrappers.
Per the repo's Python guideline to add logging for new flows, the three new decorators silently return a generic JSON error on denial without logging the agent_id/tenant_id, which makes auth failures hard to diagnose in production. Consider logging at
warninglevel when access is rejected.🪵 Suggested change
def _require_canvas_access_sync(func): `@wraps`(func) def wrapper(*args, **kwargs): - if not UserCanvasService.accessible(kwargs.get('agent_id'), kwargs.get('tenant_id')): + agent_id = kwargs.get('agent_id') + tenant_id = kwargs.get('tenant_id') + if not UserCanvasService.accessible(agent_id, tenant_id): + logging.warning("Canvas access denied: agent_id=%s tenant_id=%s", agent_id, tenant_id) return get_json_result(data=False, message="Make sure you have permission to access the agent.", code=RetCode.OPERATING_ERROR) return func(*args, **kwargs) return wrapper def _require_canvas_access_async(func): `@wraps`(func) async def wrapper(*args, **kwargs): - if not UserCanvasService.accessible(kwargs.get('agent_id'), kwargs.get('tenant_id')): + agent_id = kwargs.get('agent_id') + tenant_id = kwargs.get('tenant_id') + if not UserCanvasService.accessible(agent_id, tenant_id): + logging.warning("Canvas access denied: agent_id=%s tenant_id=%s", agent_id, tenant_id) return get_json_result(data=False, message="Make sure you have permission to access the agent.", code=RetCode.OPERATING_ERROR) return await func(*args, **kwargs) return wrapper def _require_canvas_owner_sync(func): `@wraps`(func) def wrapper(*args, **kwargs): - if not UserCanvasService.query(user_id=kwargs.get('tenant_id'), id=kwargs.get('agent_id')): + agent_id = kwargs.get('agent_id') + tenant_id = kwargs.get('tenant_id') + if not UserCanvasService.query(user_id=tenant_id, id=agent_id): + logging.warning("Canvas owner-only operation denied: agent_id=%s tenant_id=%s", agent_id, tenant_id) return get_json_result(data=False, message="Only the owner of the agent is authorized for this operation.", code=RetCode.OPERATING_ERROR) return func(*args, **kwargs) return wrapperAs per coding guidelines: "Add logging for new flows".
🤖 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 `@api/apps/restful_apis/agent_api.py` around lines 71 - 95, The three decorators (_require_canvas_access_sync, _require_canvas_access_async, _require_canvas_owner_sync) must log authorization denials before returning the JSON error; add a warning-level log (using the module logger) in each wrapper that records the agent_id and tenant_id and a short reason (e.g., "access denied" for UserCanvasService.accessible failures and "owner required" for UserCanvasService.query failures) immediately before the get_json_result return, ensuring the async wrapper logs similarly before awaiting the wrapped function.
🧹 Nitpick comments (1)
api/apps/restful_apis/agent_api.py (1)
71-95: 💤 Low valueDecorators rely on URL/query kwargs containing both
agent_idandtenant_id.Both
kwargs.get('agent_id')andkwargs.get('tenant_id')silently fall back toNoneif either key is missing.accessible(None, None)/query(user_id=None, id=None)will likely just return falsy, producing a generic permission denied message even when the real cause is a misconfigured route or a missing@add_tenant_id_to_kwargs. Consider asserting/raising a 500 (or logging an error) when these are missing so route-misconfiguration regressions are caught loudly rather than masked as 403s.This is also relevant given Line 808's chat-completion route: previously this decorator was incorrectly applied where
agent_idis not a URL kwarg; an explicit guard would have made that bug obvious.🤖 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 `@api/apps/restful_apis/agent_api.py` around lines 71 - 95, The decorators _require_canvas_access_sync, _require_canvas_access_async and _require_canvas_owner_sync should explicitly check for presence of both 'agent_id' and 'tenant_id' in kwargs before calling UserCanvasService; update each wrapper to if either kwargs.get('agent_id') is None or kwargs.get('tenant_id') is None then log an error about a misconfigured route/missing `@add_tenant_id_to_kwargs` and return get_json_result(data=False, message="Missing route parameters: agent_id or tenant_id (possible misconfigured route).", code=RetCode.SERVER_ERROR) so missing URL/query args produce a loud 500 rather than a silent permission-denied 403; otherwise proceed with the existing accessible/query checks (preserve async await in _require_canvas_access_async).
🤖 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 `@api/apps/restful_apis/agent_api.py`:
- Around line 80-86: The async decorator _require_canvas_access_async calls the
synchronous UserCanvasService.accessible inline and blocks the event loop;
change it to offload that DB call via await
thread_pool_exec(UserCanvasService.accessible, agent_id, tenant_id) (extract
agent_id and tenant_id from kwargs as currently done), check the returned
boolean and return get_json_result(data=False, message="Make sure you have
permission to access the agent.", code=RetCode.OPERATING_ERROR) when false,
otherwise await and return func(*args, **kwargs); keep the decorator name and
wrapper signature the same.
---
Outside diff comments:
In `@api/apps/restful_apis/agent_api.py`:
- Around line 71-95: The three decorators (_require_canvas_access_sync,
_require_canvas_access_async, _require_canvas_owner_sync) must log authorization
denials before returning the JSON error; add a warning-level log (using the
module logger) in each wrapper that records the agent_id and tenant_id and a
short reason (e.g., "access denied" for UserCanvasService.accessible failures
and "owner required" for UserCanvasService.query failures) immediately before
the get_json_result return, ensuring the async wrapper logs similarly before
awaiting the wrapped function.
---
Nitpick comments:
In `@api/apps/restful_apis/agent_api.py`:
- Around line 71-95: The decorators _require_canvas_access_sync,
_require_canvas_access_async and _require_canvas_owner_sync should explicitly
check for presence of both 'agent_id' and 'tenant_id' in kwargs before calling
UserCanvasService; update each wrapper to if either kwargs.get('agent_id') is
None or kwargs.get('tenant_id') is None then log an error about a misconfigured
route/missing `@add_tenant_id_to_kwargs` and return get_json_result(data=False,
message="Missing route parameters: agent_id or tenant_id (possible misconfigured
route).", code=RetCode.SERVER_ERROR) so missing URL/query args produce a loud
500 rather than a silent permission-denied 403; otherwise proceed with the
existing accessible/query checks (preserve async await in
_require_canvas_access_async).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f13f2371-d91d-4f83-bbdf-185c2ea759d7
📒 Files selected for processing (1)
api/apps/restful_apis/agent_api.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/testcases/test_http_api/test_session_management/test_agent_sessions.py (1)
83-115: 🏗️ Heavy liftNo positive test for the core bug fix (team member editing an existing agent).
The test only exercises error-path contracts on a non-existent
"invalid-agent-id". The actual scenario fixed by this PR — a team member successfully editing an existing agent they don't own — is not covered anywhere in this file. Similarly, the restriction that team members cannot delete is only implied by the message string, not exercised with real team-member credentials against a real agent.Without a positive-path test using a second
HttpApiAuth(team-member credentials) against a validagent_id, it is possible for the underlying server-side change to be wrong (e.g., still blocking edits, or accidentally allowing deletes) while all updated assertions here still pass.Consider adding a dedicated test (or a separate class
TestTeamMemberAgentAccess) with:
PUT /agents/{real_agent_id}as a team member → expect success (code == 0)DELETE /agents/{real_agent_id}as a team member → expect failure (code == 103)🤖 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 `@test/testcases/test_http_api/test_session_management/test_agent_sessions.py` around lines 83 - 115, Add a positive-path test to cover team-member edits/deletes: create a new test (e.g., in TestAgentSessions or a new TestTeamMemberAgentAccess) that uses a second HttpApiAuth representing a team-member user (distinct from the owner HttpApiAuth) and the real agent_id fixture; perform a PUT to /api/{VERSION}/agents/{agent_id} with updated payload and assert res["code"] == 0 (edit allowed), then perform DELETE via delete_agent(teamMemberHttpApiAuth, agent_id) and assert res["code"] == 103 with the appropriate message (delete forbidden). Ensure the test references HttpApiAuth/teamMemberHttpApiAuth, agent_id, the PUT helper or direct requests.put, and delete_agent so reviewers can locate the new assertions.
🤖 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.
Nitpick comments:
In `@test/testcases/test_http_api/test_session_management/test_agent_sessions.py`:
- Around line 83-115: Add a positive-path test to cover team-member
edits/deletes: create a new test (e.g., in TestAgentSessions or a new
TestTeamMemberAgentAccess) that uses a second HttpApiAuth representing a
team-member user (distinct from the owner HttpApiAuth) and the real agent_id
fixture; perform a PUT to /api/{VERSION}/agents/{agent_id} with updated payload
and assert res["code"] == 0 (edit allowed), then perform DELETE via
delete_agent(teamMemberHttpApiAuth, agent_id) and assert res["code"] == 103 with
the appropriate message (delete forbidden). Ensure the test references
HttpApiAuth/teamMemberHttpApiAuth, agent_id, the PUT helper or direct
requests.put, and delete_agent so reviewers can locate the new assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dcbc9ecb-8e2b-4655-ad17-ad0d6d656ada
📒 Files selected for processing (1)
test/testcases/test_http_api/test_session_management/test_agent_sessions.py
The inline permission checks in update_agent and delete_agent were moved to decorators (@_require_canvas_access_async, @_require_canvas_owner_sync) in a previous commit. The tests were still using __wrapped__ to bypass decorators, so patching UserCanvasService.query had no effect. Update the tests to: - Patch UserCanvasService.accessible instead of query for update_agent - Test decorators directly via dummy functions instead of __wrapped__ - Use _require_canvas_owner_sync dummy for delete_agent permission test
1 participant
What problem does this PR solve?
Follow on PR: #14602
to fix: team member cannot edit agent.
new behavior: beside delete, everything is allowed for team member.
Type of change