-
Notifications
You must be signed in to change notification settings - Fork 50
feat(authz): register agentex resources and grant agent/task ownership #270
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,16 +76,9 @@ async def get_agent_by_name( | |
| AgentexResourceType.agent, AuthorizedOperationType.read | ||
| ), | ||
| agents_use_case: DAgentsUseCase, | ||
| authorization: DAuthorizationService, | ||
| ): | ||
| """Get an agent by its unique name.""" | ||
| agent_entity = await agents_use_case.get(name=agent_name) | ||
|
|
||
| await authorization.check( | ||
| resource=AgentexResource.agent(agent_entity.id), | ||
| operation=AuthorizedOperationType.read, | ||
| ) | ||
|
|
||
|
Comment on lines
-84
to
-88
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we removing this? Does it now happen at a higher layer?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's moved up into the parameter dependency, so the body call was a duplicate. get_agent_by_id directly above already works this way
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this was duplicative even before this PR. |
||
| return Agent.model_validate(agent_entity) | ||
|
|
||
|
|
||
|
|
@@ -183,7 +176,6 @@ async def register_agent( | |
| await authorization_service.check( | ||
| AgentexResource.agent("*"), | ||
| AuthorizedOperationType.create, | ||
| principal_context=request.principal_context, | ||
| ) | ||
| logger.info(f"Registering agent: {request}") | ||
| try: | ||
|
|
@@ -198,7 +190,6 @@ async def register_agent( | |
| ) | ||
| await authorization_service.grant( | ||
| AgentexResource.agent(agent_entity.id), | ||
| principal_context=request.principal_context, | ||
| ) | ||
| response_fields = agent_entity.model_dump() | ||
| existing_key = await api_keys_use_case.get_internal_api_key_by_agent_id( | ||
|
|
@@ -236,11 +227,10 @@ async def register_build( | |
| agents_use_case: DAgentsUseCase, | ||
| authorization_service: DAuthorizationService, | ||
| ) -> Agent: | ||
| """Create a build-only agent row and register its authz resource.""" | ||
| """Create a build-only agent row after create authorization.""" | ||
| await authorization_service.check( | ||
| AgentexResource.agent("*"), | ||
| AuthorizedOperationType.create, | ||
| principal_context=request.principal_context, | ||
| ) | ||
| logger.info(f"Registering build for agent: {request.name}") | ||
| try: | ||
|
|
@@ -252,9 +242,8 @@ async def register_build( | |
| ) | ||
| except ValueError as e: | ||
| raise HTTPException(status_code=400, detail=str(e)) from e | ||
| await authorization_service.register_resource( | ||
| await authorization_service.grant( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So we do a register resource in agentex/src/domain/use_cases/agents_use_case.py and then a grant out here. Do we need to do both? Can we keep the authz checks/grants at the router layer ideally instead of nested in the business logic? Thoughts?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we need both, they're the two backends of the dual-write, not redundant.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure makes sense but still can we move it up from the business layer? or move this call down. Ideally all authz checks and writes happen at the highest layer possible IMO
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agree on the principle for the checks. gating belongs at the edge and it already is there: the
The route only gets control after |
||
| AgentexResource.agent(agent_entity.id), | ||
| principal_context=request.principal_context, | ||
| ) | ||
| return Agent.model_validate(agent_entity) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep the authorization.check/revoke/grant. Otherwise, how can we maintain backward compatibility?
1, removing authorization.check will remove authz altogether.
2, removing grant/revoke will break the old authz even if the user opts out of the new FGAC
Even if a user turns FGAC on, we may still want to keep grant/revoke for permission data dual-write, so that the old authz still works in case the user turns FGAC off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
went and traced this through
agentex-authto make sure we're solid, and I don't think we need to keep grant/revoke heretl;dr: on the legacy (SGP) backend,
register_resourceisgrantandderegister_resourceisrevoke. so dropping the explicit grant/revoke and going through register/deregister is behavior-preserving for old authz, not a regression.it doesn't. the only
checkI removed is the explicit one inget_agent_by_name, but that route still hasagent_name: DAuthorizedName(agent, read)in its signature, and that dependency callsauthorization.check(agent, read)itself before the handler body runs (authorization_shortcuts.py). so the line I deleted was a redundant second check of the same tuple. read authz is still enforced. on create I kept thecheck(agent("*"), create)too, just dropped the unusedprincipal_context=kwarg.two things:
agentex-authroutes every call (grant/revoke/check/register/deregister) to exactly one provider per account based onFGAC_AGENTEX_AUTH_SPARK(_resolve_provider). flag off → legacy SGP, flag on → Spark. there's no write-to-both happening, it's a routing switch.register_resourcedelegates straight tograntandderegister_resourcedelegates torevokeso for an FGAC-off account,
register_resource(agent)→ SGP →grant(owner), which is exactly what the old route did.deregister_resource→revoke. legacy authz is fully preserved. for FGAC-on it does the proper Spark resource+owner write (tenant scoping comes from account_id automatically). either way it's correct, and keeping grant/revoke on top would just be a redundant duplicate write.