Fix-forward PR 2187 (consent defer): wire the flag + tests - #2308
Conversation
…ard) _do_approve never passed defer_binding to approve_request_record, so the flag added in PR 2187 was dead code: the project_id-required 400 guard still fired on the defer combo, and when project_id was present the token was bound anyway despite defer_binding:true. - Pass defer_binding=body.defer_binding at the call site. - Add a 400 guard for defer_binding:true with an explicit (non-blank) project_id -- the two are contradictory. - Route tests covering the defer combo (unbound token, unbound grants, no membership row, no a2a channel), the 400 guard, and the unchanged non-deferred path. Red run on buggy head (before the fix): test_defer_with_project_scopes_no_project_id_succeeds_unbound -> 400 (expected 200) test_defer_with_explicit_project_id_returns_400 -> 200 (expected 400) Lifecycle note on never-bound deferred identities: An unbound deferred identity is minted with project_id=None on both the token and every grant. It is currently indistinguishable from a global identity: no expiry, no pending binding flag, no visibility surface in the Permissions app. A follow-up card should track (a) a lifecycle/expiry mechanism so deferred-but-never-bound grants do not persist forever, and (b) a visibility surface so operators can see and reap stale deferred identities.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe approval API now supports deferred project binding. Deferred approvals issue unbound tokens and grants, skip project membership and A2A setup, and reject explicit project IDs. Standard project approval behavior remains unchanged. ChangesDeferred Binding Approval
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant _do_approve
participant approve_request_record
participant TokenAndGrantMinting
participant ProjectMembershipAndA2A
Client->>_do_approve: Submit approval with defer_binding
_do_approve->>approve_request_record: Forward approval request
approve_request_record->>TokenAndGrantMinting: Mint unbound token and grant
approve_request_record->>ProjectMembershipAndA2A: Skip synchronization without binding_project
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
PR Summary by QodoFix defer_binding approval flow and add route tests
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Reviewed by step-3.7-flash · Input: 117.5K · Output: 20K · Cached: 232.3K |
|
nemotron-super review VERDICT: No blocking issues found. Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
Code Review by Qodo
1. Deferred scopes become OS-level
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| # When deferring, the token and grants are minted unbound (project_id=None) | ||
| # regardless of effective_project; project-scoped calls 403 until the agent | ||
| # is bound to a project later via assign-agent. | ||
| binding_project = None if defer_binding else effective_project |
There was a problem hiding this comment.
1. Deferred scopes become os-level 🐞 Bug ⛨ Security
When defer_binding=true, approve_request_record sets binding_project=None and uses it for every grant, so global-capable scopes like decisions_write/decisions_read are stored with project_id=NULL (OS-level) instead of being project-bound. This means an approval that includes decisions_* will authorize OS-level decisions routes (project_id=None) while project binding is deferred.
Agent Prompt
## Issue description
`defer_binding=true` currently forces `project_id=None` for **all** approved scopes, which makes global-capable scopes (e.g. `decisions_read`/`decisions_write`) become OS-level grants. If `defer_binding` is intended to defer only *project binding* for strictly project-scoped scopes, this should be prevented or made explicit.
## Issue Context
- `binding_project = None if defer_binding else effective_project` is applied uniformly.
- Grants for scopes like `decisions_write` treat `project_id=None` as OS-level authorization.
## Fix Focus Areas
- tinyagentos/routes/agent_auth_requests.py[428-440]
- tinyagentos/routes/agent_auth_requests.py[564-580]
- tinyagentos/routes/agent_auth_requests.py[826-843]
### Suggested implementation direction
- Add a guard in `_do_approve` (or `approve_request_record`) that when `defer_binding=True`, the `granted_scopes` must be a subset of `_PROJECT_SCOPES` (or another explicitly-allowed deferred set).
- Alternatively, split binding behavior per-scope: keep project-scoped scopes unbound, but require explicit validated `project_id` for any global-capable scope that would otherwise become OS-level under deferral.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Reviewed. The core is sound and the security-critical claim in the docstring is true — I verified it rather than trusting it. Two things to fix before merge, one of which is the red CI, plus one question. Verified good, stated explicitly because it is the part that could have been a privilege escalation. The docstring says deferred grants are minted unbound and "project-scoped calls 403 until assign-agent binds it". A grant row with
So deferral authorizes nothing, as claimed. Good, and worth the trace. 1. doc-gate is legitimately red, not a flakeThe CI log is unavailable (0 bytes), so I reproduced it locally against Both are correct for this change: 2. Deferring against an existing ACTIVE handle changes behaviour, is untested, and the error steers the wrong wayThe guard became: if not defer_binding and project_id and set(granted_scopes) & _PROJECT_SCOPES:So with The 409 is defensible — deferring names no project, so there is nothing to add the identity to. The remedy in the message is not. "Pick a different identity_claim" tells the operator to mint a second identity for the same agent, which is the outcome the multi-project model (#1862) exists to prevent. Someone following that advice creates exactly the duplicate this code was written to avoid. Please either give this path its own message ("this handle already has an active identity; approve with an explicit project_id to add it to a project, or bind later via assign-agent") or state deliberately that deferring on an existing handle is unsupported. Three defer tests were added and none covers this: 3. Question: the contradiction guard is asymmetricif body.defer_binding and body.project_id and body.project_id.strip():
raise HTTPException(400, "defer_binding cannot be combined with an explicit project_id")The comment reasons that naming a project while deferring is a mistake worth rejecting up front. Agreed. But Same operator intent, two different outcomes, and the silent one is the easier to hit. Is that deliberate? If deferring should always ignore a record-supplied project, a line saying so would help; if not, the guard should consider Minor, not blocking
Fix 1 and 2 and I am happy to merge. The unbound-grant design is right and the comments explaining why each |
jaylfc
added
the
lead-blocked
1 participant
CARD TITLE (intent, not commit subject): Fix-forward PR 2187 (consent defer): wire the flag + tests
Autonomous build of board card tsk-5tj7qq.
_do_approve never passed defer_binding to approve_request_record, so the
flag added in PR 2187 was dead code: the project_id-required 400 guard
still fired on the defer combo, and when project_id was present the token
was bound anyway despite defer_binding:true.
project_id -- the two are contradictory.
no membership row, no a2a channel), the 400 guard, and the unchanged
non-deferred path.
Red run on buggy head (before the fix):
test_defer_with_project_scopes_no_project_id_succeeds_unbound -> 400 (expected 200)
test_defer_with_explicit_project_id_returns_400 -> 200 (expected 400)
Lifecycle note on never-bound deferred identities:
An unbound deferred identity is minted with project_id=None on both the
token and every grant. It is currently indistinguishable from a global
identity: no expiry, no pending binding flag, no visibility surface in
the Permissions app. A follow-up card should track (a) a lifecycle/expiry
mechanism so deferred-but-never-bound grants do not persist forever, and
(b) a visibility surface so operators can see and reap stale deferred
identities.
Files:
tests/test_routes_agent_auth_requests.py | 239 ++++++++++++++++++++++++++++++
tinyagentos/routes/agent_auth_requests.py | 61 ++++++--
2 files changed, 286 insertions(+), 14 deletions(-)
Summary by CodeRabbit
New Features
Bug Fixes