Skip to content

feat: Add client headers to Toolbox #178

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

Merged
merged 39 commits into from
Apr 22, 2025
Merged

feat: Add client headers to Toolbox #178

merged 39 commits into from
Apr 22, 2025

Conversation

twishabansal
Copy link
Contributor

@twishabansal twishabansal commented Apr 17, 2025

Added a Client Headers feature, which allows users to bind certain headers to the client. These headers will be used in every subsequent request sent through the client.

  • Added an add_headers method to ToolboxClient.
  • Added optional headers in ToolboxClient init.

This would allow us to deploy Toolbox Server securely to various deployment environments like Cloud Run and connect using Authorization.

For more information, see Toolbox Cloud Run Authentication.

Note: I'll add the same feature in the Sync Client in a following PR.

# Conflicts:
#	packages/toolbox-core/src/toolbox_core/client.py
#	packages/toolbox-core/src/toolbox_core/tool.py
@twishabansal twishabansal changed the title Add headers feat: Add client headers to Toolbox Apr 17, 2025
@twishabansal twishabansal requested a review from a team as a code owner April 21, 2025 13:22
Copy link
Contributor

@kurtisvg kurtisvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small nit for next time: given the size of the PR, it would have been better to introduce the refactoring to use utility functions in a different PR first.

Co-authored-by: Kurtis Van Gent <31518063+kurtisvg@users.noreply.github.com>
@twishabansal
Copy link
Contributor Author

small nit for next time: given the size of the PR, it would have been better to introduce the refactoring to use utility functions in a different PR first.

This has been done in another PR: ca88369. I just edited the import statement. However, it's interesting that even with the wrong import in toolbox-core at this commit, the tests passed. I'm looking into it.

I replicated this PR locally and no tests fail.
In the file client.py, we have this import statement:

from .tool import ToolboxTool, identify_required_authn_params

but identify_required_authn_params is present in the utils file.

@anubhav756 do you have any ideas why?

@twishabansal twishabansal merged commit e73f6b6 into main Apr 22, 2025
14 checks passed
@twishabansal twishabansal deleted the add-headers-poc branch April 22, 2025 07:23
@release-please release-please bot mentioned this pull request Apr 21, 2025
anubhav756 added a commit that referenced this pull request May 5, 2025
Post adding the feature of adding client-level auth headers (#178), we have the logic for creating an auth header, from the given auth token getter name, in 3 different places.

This PR unifies all of that logic into a single helper to improve maintenance, and make it easier to change the way we add suffix/prefix, and reduces WET code.
anubhav756 added a commit that referenced this pull request May 5, 2025
Post adding the feature of adding client-level auth headers (#178), we have the logic for creating an auth header, from the given auth token getter name, in 3 different places.

This PR unifies all of that logic into a single helper to improve maintenance, and make it easier to change the way we add suffix/prefix, and reduces WET code.
anubhav756 added a commit that referenced this pull request May 5, 2025
Post adding the feature of adding client-level auth headers (#178), we have the logic for creating an auth header, from the given auth token getter name, in 3 different places.

This PR unifies all of that logic into a single helper to improve maintenance, and make it easier to change the way we add suffix/prefix, and reduces WET code.
anubhav756 added a commit that referenced this pull request May 12, 2025
Post adding the feature of adding client-level auth headers (#178), we have the logic for creating an auth header, from the given auth token getter name, in 3 different places.

This PR unifies all of that logic into a single helper to improve maintenance, and make it easier to change the way we add suffix/prefix, and reduces WET code.
anubhav756 added a commit that referenced this pull request May 12, 2025
* chore: Add unit test coverage.

* chore: Consolidate auth header creation logic

Post adding the feature of adding client-level auth headers (#178), we have the logic for creating an auth header, from the given auth token getter name, in 3 different places.

This PR unifies all of that logic into a single helper to improve maintenance, and make it easier to change the way we add suffix/prefix, and reduces WET code.

* chore: Delint

* chore: Delint
anubhav756 added a commit that referenced this pull request May 12, 2025
Post adding the feature of adding client-level auth headers (#178), we have the logic for creating an auth header, from the given auth token getter name, in 3 different places.

This PR unifies all of that logic into a single helper to improve maintenance, and make it easier to change the way we add suffix/prefix, and reduces WET code.
anubhav756 added a commit that referenced this pull request May 12, 2025
…istency (#219)

* chore: Add unit test coverage.

* chore: Consolidate auth header creation logic

Post adding the feature of adding client-level auth headers (#178), we have the logic for creating an auth header, from the given auth token getter name, in 3 different places.

This PR unifies all of that logic into a single helper to improve maintenance, and make it easier to change the way we add suffix/prefix, and reduces WET code.

* chore(toolbox-core): Standardize on ValueError for improved error consistency

This PR updates instances where generic `Exception` objects were thrown in `toolbox-core` to use `ValueError` instead.

This aligns with the prevalent use of `ValueError` for argument-related issues elsewhere in the package, leading to more specific, predictable, and robust error handling for consumers of `toolbox-core`.

* chore: Delint

* chore: Remove duplicate test
anubhav756 added a commit that referenced this pull request May 12, 2025
Post adding the feature of adding client-level auth headers (#178), we have the logic for creating an auth header, from the given auth token getter name, in 3 different places.

This PR unifies all of that logic into a single helper to improve maintenance, and make it easier to change the way we add suffix/prefix, and reduces WET code.
anubhav756 added a commit that referenced this pull request May 12, 2025
* chore: Add unit test coverage.

* chore: Consolidate auth header creation logic

Post adding the feature of adding client-level auth headers (#178), we have the logic for creating an auth header, from the given auth token getter name, in 3 different places.

This PR unifies all of that logic into a single helper to improve maintenance, and make it easier to change the way we add suffix/prefix, and reduces WET code.

* docs: Add names to return values for better readability

* chore: Improve readability

* chore: Remove redundant tests

* feat: Introduce identifying used authz token getters

This PR adds the feature in `identify_required_authn_params` helper to determine which of the provided auth token getters are actively used to satisfy a tool's authorization requirements (as defined by the `authRequired` key in its manifest).

This is a foundational step towards future validation in `ToolboxTool.add_auth_token_getters`, which will ensure no configured auth token getters remain unused, thereby preventing potential misconfigurations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants