Skip to content

Commit 74ddda7

Browse files
committed
fix: Add validation to ensure added auth token getters are used by the tool
### Problem Previously, the `ToolboxTool.add_auth_token_getters` method only validated against existing registered getters or conflicts with client headers. It did not verify if *all* the auth token getters provided were actually used or required by the specific tool instance they were being added to. ### Solution This PR enhances the validation in `add_auth_token_getters`. It now leverages the `used_auth_token_getters` information returned by the existing `identify_required_authn_params` call. This allows the method to confirm that every getter passed in is genuinely required by the tool, raising an error if any are unused. ### Benefit This ensures that only relevant auth token getters are attempted to be registered for a tool, preventing misconfigurations and human errors. > [!NOTE] > This validation aligns with the existing validation logic already present in the `ToolboxClient.load_tool` method, promoting a consistent approach to handling auth token getter requirements across the codebase.
1 parent 4f0feca commit 74ddda7

File tree

2 files changed

+50
-7
lines changed

2 files changed

+50
-7
lines changed

packages/toolbox-core/src/toolbox_core/tool.py

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -268,16 +268,23 @@ def add_auth_token_getters(
268268
new_getters = types.MappingProxyType(
269269
dict(self.__auth_service_token_getters, **auth_token_getters)
270270
)
271-
# create a read-only updated for params that are still required
272-
new_req_authn_params = types.MappingProxyType(
273-
identify_required_authn_params(
274-
self.__required_authn_params, auth_token_getters.keys()
275-
)[0]
271+
272+
# find the updated required authn params and the auth token getters used
273+
new_req_authn_params, used_auth_token_getters = identify_required_authn_params(
274+
self.__required_authn_params, auth_token_getters.keys()
276275
)
277276

277+
# ensure no auth token getter provided remains unused
278+
unused_auth = set(incoming_services) - used_auth_token_getters
279+
if unused_auth:
280+
raise ValueError(
281+
f"Authentication source(s) `{', '.join(unused_auth)}` unused by tool `{self.__name__}`."
282+
)
283+
278284
return self.__copy(
279285
auth_service_token_getters=new_getters,
280-
required_authn_params=new_req_authn_params,
286+
# create a read-only updated for params that are still required
287+
required_authn_params=types.MappingProxyType(new_req_authn_params),
281288
)
282289

283290
def bind_params(

packages/toolbox-core/tests/test_tool.py

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,10 @@
1111
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
1212
# See the License for the specific language governing permissions and
1313
# limitations under the License.
14+
15+
1416
import inspect
15-
from typing import AsyncGenerator, Callable
17+
from typing import AsyncGenerator, Callable, Mapping
1618
from unittest.mock import AsyncMock, Mock
1719

1820
import pytest
@@ -92,6 +94,12 @@ def auth_header_key() -> str:
9294
return "test-auth_token"
9395

9496

97+
@pytest.fixture
98+
def unused_auth_getters() -> dict[str, Callable[[], str]]:
99+
"""Provides an auth getter for a service not required by sample_tool."""
100+
return {"unused-auth-service": lambda: "unused-token-value"}
101+
102+
95103
def test_create_func_docstring_one_param_real_schema():
96104
"""
97105
Tests create_func_docstring with one real ParameterSchema instance.
@@ -426,3 +434,31 @@ def test_tool_add_auth_token_getters_conflict_with_existing_client_header(
426434

427435
with pytest.raises(ValueError, match=expected_error_message):
428436
tool_instance.add_auth_token_getters(new_auth_getters_causing_conflict)
437+
438+
439+
def test_add_auth_token_getters_unused_token(
440+
http_session: ClientSession,
441+
sample_tool_params: list[ParameterSchema],
442+
sample_tool_description: str,
443+
unused_auth_getters: Mapping[str, Callable[[], str]],
444+
):
445+
"""
446+
Tests ValueError when add_auth_token_getters is called with a getter for
447+
an unused authentication service.
448+
"""
449+
tool_instance = ToolboxTool(
450+
session=http_session,
451+
base_url=TEST_BASE_URL,
452+
name=TEST_TOOL_NAME,
453+
description=sample_tool_description,
454+
params=sample_tool_params,
455+
required_authn_params={},
456+
auth_service_token_getters={},
457+
bound_params={},
458+
client_headers={},
459+
)
460+
461+
expected_error_message = "Authentication source\(s\) \`unused-auth-service\` unused by tool \`sample_tool\`."
462+
463+
with pytest.raises(ValueError, match=expected_error_message):
464+
tool_instance.add_auth_token_getters(unused_auth_getters)

0 commit comments

Comments
 (0)