-
Notifications
You must be signed in to change notification settings - Fork 23
feat: Add #[IsGrantedTool] for tool access control
#382
Conversation
32b5a03 to
8a0aa6c
Compare
4504d93 to
d75c01e
Compare
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.
Can we use the Symfony one instead? it's basically the same code besides Tool as a subject - but I can't spot where you need that
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.
It would mean requiring symfony/security-http instead of just symfony/security-core, and that pulls in a lot of dependencies, f.e. symfony/http-foundation and symfony/http-kernel.
Of course since it's an optional dependency, maybe that's ok? And in practice I think most people use this library with the bundle & likely have full Symfony setup anyway so 🤷
it's basically the same code besides Tool as a subject
it's about what arguments the Closure gets. If we use the built-in IsGranted, then should we provide Request object to the callback like the typehint says?
Also it could be misleading that an expression in a single #[IsGranted] attribute would get different parameters for evaluation depending on the context:
- symfony http request:
['request' => ...,' 'args' => ...] - php-llm tool call:
['tool' => ...,' 'args' => ...]
Do you have a take on this @OskarStark ?
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.
alright, yea, reusing the "original" one is not a great idea - you're right.
it's just confusing since I compared it to the other one and the controller is also not part of the attribute payload, but if you're sure the tool itself should be part of the payload - fair enough. 👍
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.
well, the controller is part of the request's attributes
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 guess more analogous to Request would be a ToolCall instance, but for some reason I didn't include it in the ToolCallArgumentsResolved event 🤔
EDIT: the reason was that the event already contains the denormalized arguments & tool metadata, and I didn't see a point of including a structure that includes normalized args in addition
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 would also not reuse it and maybe use a more tailored name like IsGrantedTool ?
|
A slight security concern: this library does not depend on |
|
If the class is not available we can throw an exception, no? "For using.... EventDispatcher is needed, Try running composer require ...." like Symfony is doing it |
|
I tried adding the check directly to the top of the Can't think of any place except the toolbox itself where you could check it, and if you do, then it kinda defeats the purpose of using events to decouple logic/domains |
#[IsGranted] for tool access control#[IsGrantedTool] for tool access control
|
We have the same issue with the I feel like this feature is relying quite a lot on the infrastructure the Symfony framework is providing and therefore should go to the bundle instead. WDYT? |
Sounds good to me |
…l (valtzu) This PR was merged into the main branch. Discussion ---------- [AIBundle] Add `#[IsGrantedTool]` for tool access control | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | Docs? | yes | Issues | Fix php-llm/llm-chain#360 | License | MIT Add `#[IsGrantedTool]` attribute for tool access control with similar behavior as `#[IsGranted]` in `symfony/security-http`. Moved from php-llm/llm-chain#382 Commits ------- 40761da [AIBundle] Add `#[IsGrantedTool]` for tool access control
…l (valtzu) This PR was merged into the main branch. Discussion ---------- [AIBundle] Add `#[IsGrantedTool]` for tool access control | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | Docs? | yes | Issues | Fix php-llm/llm-chain#360 | License | MIT Add `#[IsGrantedTool]` attribute for tool access control with similar behavior as `#[IsGranted]` in `symfony/security-http`. Moved from php-llm/llm-chain#382 Commits ------- 40761da [AIBundle] Add `#[IsGrantedTool]` for tool access control
Resolves #360