From c849ff6eeca701048b228900e9aa89419f039831 Mon Sep 17 00:00:00 2001 From: Kunal Karmakar Date: Mon, 1 Jun 2026 14:03:43 +0000 Subject: [PATCH] Address PR review comments --- nanobot/webui/settings_api.py | 6 +- tests/providers/test_azure_openai_provider.py | 53 +++++++- tests/webui/test_settings_api.py | 118 +++++++++++++++++- 3 files changed, 171 insertions(+), 6 deletions(-) diff --git a/nanobot/webui/settings_api.py b/nanobot/webui/settings_api.py index d476cb70b..d4c8849c9 100644 --- a/nanobot/webui/settings_api.py +++ b/nanobot/webui/settings_api.py @@ -245,8 +245,8 @@ def _resolve_env_placeholders(value: str | None) -> str | None: def _provider_requires_api_key(spec: Any) -> bool: - if spec.backend == "azure_openai": - return True + if spec.name == "azure_openai": + return False if spec.is_oauth: return False if spec.is_local or spec.is_direct: @@ -305,6 +305,8 @@ def _oauth_provider_status(spec: Any) -> dict[str, Any]: def _provider_configured_for_settings(spec: Any, provider_config: Any) -> bool: if spec.is_oauth: return bool(_oauth_provider_status(spec)["configured"]) + if spec.name == "azure_openai": + return bool(provider_config.api_base) if _provider_requires_api_key(spec): return bool(provider_config.api_key) return bool( diff --git a/tests/providers/test_azure_openai_provider.py b/tests/providers/test_azure_openai_provider.py index 73d078d2e..c44cc3eee 100644 --- a/tests/providers/test_azure_openai_provider.py +++ b/tests/providers/test_azure_openai_provider.py @@ -93,10 +93,57 @@ def test_init_missing_key_uses_aad_token_provider(monkeypatch): assert isinstance(provider._token_provider, _AzureTokenProvider) # DefaultAzureCredential must have been instantiated exactly once credential_factory.assert_called_once_with() - # The SDK client must have received the token provider as its api_key - # (the SDK stores it on the auth wrapper, not directly accessible — so - # we assert the callable was wired in via the provider attribute). assert provider._token_provider._credential is credential_instance + # The token provider must be wired into the OpenAI SDK as its + # ``_api_key_provider`` callable — that's what the SDK invokes per + # request to refresh the bearer token. + assert provider._client._api_key_provider is provider._token_provider + # Static api_key starts empty until the first refresh. + assert provider._client.api_key == "" + + +@pytest.mark.asyncio +async def test_aad_token_provider_wires_into_sdk_auth_headers(monkeypatch): + """End-to-end: SDK ``_refresh_api_key`` invokes our callable and the + resulting bearer token shows up in ``auth_headers``. + + This is a regression guard against a future refactor that constructs + ``_AzureTokenProvider`` but forgets to pass it to ``AsyncOpenAI`` (in + which case the outgoing request would carry no Authorization header). + """ + access_token = SimpleNamespace(token="token-A", expires_on=time.time() + 3600) + credential_instance = MagicMock() + credential_instance.get_token = AsyncMock(return_value=access_token) + credential_factory = MagicMock(return_value=credential_instance) + _install_fake_azure_identity(monkeypatch, credential_factory) + + provider = AzureOpenAIProvider( + api_key="", api_base="https://res.openai.azure.com", + ) + + # Before any refresh, the SDK has no key yet -> no auth header. + assert provider._client.api_key == "" + assert provider._client.auth_headers == {} + + # Trigger the SDK's refresh path; it must call our async callable. + refreshed = await provider._client._refresh_api_key() + + assert refreshed == "token-A" + assert provider._client.api_key == "token-A" + assert provider._client.auth_headers == {"Authorization": "Bearer token-A"} + credential_instance.get_token.assert_awaited_with( + "https://cognitiveservices.azure.com/.default" + ) + + # A second refresh picks up a rotated token without re-instantiating + # the credential — proves we delegate per-request rather than caching + # the first value. + credential_instance.get_token = AsyncMock( + return_value=SimpleNamespace(token="token-B", expires_on=time.time() + 3600) + ) + await provider._client._refresh_api_key() + assert provider._client.auth_headers == {"Authorization": "Bearer token-B"} + credential_factory.assert_called_once_with() def test_init_explicit_key_does_not_construct_credential(monkeypatch): diff --git a/tests/webui/test_settings_api.py b/tests/webui/test_settings_api.py index 470b23bba..80a56ed91 100644 --- a/tests/webui/test_settings_api.py +++ b/tests/webui/test_settings_api.py @@ -7,6 +7,7 @@ import pytest from nanobot.config.loader import load_config, save_config from nanobot.config.schema import Config, ModelPresetConfig +from nanobot.providers.registry import find_by_name from nanobot.webui.settings_api import ( WebUISettingsError, _oauth_provider_status, @@ -17,7 +18,6 @@ from nanobot.webui.settings_api import ( update_model_configuration, update_network_safety_settings, ) -from nanobot.providers.registry import find_by_name def test_create_model_configuration_writes_label_and_selects( @@ -461,3 +461,119 @@ def test_create_model_configuration_accepts_configured_oauth_provider( assert payload["agent"]["model_preset"] == "codex" saved = load_config(config_path) assert saved.model_presets["codex"].provider == "openai_codex" + + +# --------------------------------------------------------------------------- +# Azure OpenAI: settings contract for static-key vs AAD (DefaultAzureCredential) +# --------------------------------------------------------------------------- + + +def test_settings_payload_azure_openai_with_api_key_is_configured( + tmp_path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Static-key mode: api_key + api_base both set -> configured.""" + config_path = tmp_path / "config.json" + config = Config() + config.providers.azure_openai.api_key = "k" + config.providers.azure_openai.api_base = "https://r.openai.azure.com" + save_config(config, config_path) + monkeypatch.setattr("nanobot.config.loader._current_config_path", config_path) + + payload = settings_payload() + azure = next(row for row in payload["providers"] if row["name"] == "azure_openai") + + assert azure["configured"] is True + assert azure["api_key_required"] is False + assert azure["auth_type"] == "api_key" + assert azure["api_base"] == "https://r.openai.azure.com" + + +def test_settings_payload_azure_openai_aad_mode_is_configured( + tmp_path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """AAD mode: only api_base set (no api_key) -> still configured.""" + config_path = tmp_path / "config.json" + config = Config() + config.providers.azure_openai.api_base = "https://r.openai.azure.com" + save_config(config, config_path) + monkeypatch.setattr("nanobot.config.loader._current_config_path", config_path) + + payload = settings_payload() + azure = next(row for row in payload["providers"] if row["name"] == "azure_openai") + + assert azure["configured"] is True + assert azure["api_key_required"] is False + assert azure["api_base"] == "https://r.openai.azure.com" + assert azure["api_key_hint"] is None + + +def test_settings_payload_azure_openai_missing_base_not_configured( + tmp_path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """api_key alone (no api_base) is NOT a working config -> not configured.""" + config_path = tmp_path / "config.json" + config = Config() + config.providers.azure_openai.api_key = "k" + save_config(config, config_path) + monkeypatch.setattr("nanobot.config.loader._current_config_path", config_path) + + payload = settings_payload() + azure = next(row for row in payload["providers"] if row["name"] == "azure_openai") + + assert azure["configured"] is False + + +def test_create_model_configuration_accepts_azure_openai_aad_mode( + tmp_path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Provider-validation accepts azure_openai with only api_base (AAD mode).""" + config_path = tmp_path / "config.json" + config = Config() + config.providers.azure_openai.api_base = "https://r.openai.azure.com" + save_config(config, config_path) + monkeypatch.setattr("nanobot.config.loader._current_config_path", config_path) + + payload = create_model_configuration( + { + "label": ["Azure AAD"], + "provider": ["azure_openai"], + "model": ["my-deployment"], + } + ) + + assert payload["agent"]["model_preset"] == "azure-aad" + saved = load_config(config_path) + assert saved.model_presets["azure-aad"].provider == "azure_openai" + assert saved.model_presets["azure-aad"].model == "my-deployment" + + +def test_create_model_configuration_rejects_azure_openai_without_base( + tmp_path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """azure_openai without api_base must still be rejected as not configured.""" + config_path = tmp_path / "config.json" + save_config(Config(), config_path) + monkeypatch.setattr("nanobot.config.loader._current_config_path", config_path) + + with pytest.raises(WebUISettingsError, match="provider is not configured"): + create_model_configuration( + { + "label": ["Azure"], + "provider": ["azure_openai"], + "model": ["my-deployment"], + } + ) + + +def test_azure_openai_spec_no_longer_requires_api_key() -> None: + """Contract guard: api_key is optional for azure_openai (AAD fallback).""" + from nanobot.webui.settings_api import _provider_requires_api_key + + spec = find_by_name("azure_openai") + assert spec is not None + assert _provider_requires_api_key(spec) is False