[FEAT]: GUI: Adding Entra Auth Option to New Targets#1762
Conversation
jbolor21
changed the title
romanlutz
left a comment
romanlutz
left a comment
There was a problem hiding this comment.
Nice work scoping Entra to OpenAI and AzureML targets explicitly. A few things — most importantly a token-leak risk on the AzureML branch, plus the frontend currently assumes every entry in TARGET_TYPE_CONFIG is Entra-capable, which won't hold as we add more target types.
| return new_params | ||
|
|
||
| if issubclass(target_class, AzureMLChatTarget): | ||
| new_params["api_key"] = get_azure_async_token_provider(_AZURE_ML_SCOPE) |
There was a problem hiding this comment.
Security: the AzureML branch has no endpoint validation, but the OpenAI branch above does. If a user submits e.g. https://attacker.example.com/score as the AML endpoint with auth_mode="entra", we'll happily mint an https://ml.azure.com/.default bearer token here and AzureMLChatTarget._get_headers_async will then send it in the Authorization header to the attacker's URL on every request.
Please add a strict hostname-suffix check for AML endpoints (e.g. *.inference.ml.azure.com) analogous to _is_azure_openai_endpoint, and raise before injecting the provider if it doesn't match. The frontend warning should mirror this so the asymmetry is gone end to end.
| value={apiKey} | ||
| onChange={(_, data) => setApiKey(data.value)} | ||
| /> | ||
| <Field label="Authentication"> |
There was a problem hiding this comment.
This Authentication field is rendered unconditionally — the Entra radio shows for every entry in TARGET_TYPE_CONFIG, and even before any type is picked. Today that's fine because all 7 entries are Entra-capable, but the moment someone adds a non-Azure target (HuggingFace, Ollama, etc.) to the dropdown, picking Entra → Submit will hit the backend's "does not support Entra" raise and surface as a confusing 500.
I'd make Entra capability an explicit per-type property rather than an implicit assumption:
interface TargetTypeConfig { kind: 'openai' | 'azureml'; supportsEntra: boolean }
const TARGET_TYPE_CONFIG: Record<string, TargetTypeConfig> = {
OpenAIChatTarget: { kind: 'openai', supportsEntra: true },
// ...
AzureMLChatTarget: { kind: 'azureml', supportsEntra: true },
}Then hide the whole Authentication field (and fall back to the plain API Key input) when !targetConfig?.supportsEntra. Also worth gating it on targetType !== '' so we don't show auth controls before a type is selected.
| </Field> | ||
|
|
||
| {showNonAzureEntraWarning && ( | ||
| <MessageBar intent="warning" className={styles.warningMessage}> |
There was a problem hiding this comment.
Two things on this warning block:
- The MessageBar is
intent="warning"but the body text starts with"Error:". Pick one — either drop theError:prefix or switch tointent="error". - The warning is purely informational —
disabled={submitting || !targetType || !endpoint}on the Create Target button doesn't account for it, so the user can still click Create and get a server round-trip failure. Either includeshowNonAzureEntraWarningin the disabled condition, or surface this as avalidationState="error"on the Endpoint field so submission is blocked the same way other field errors are.
| auth_mode: Literal["api_key", "entra"] = Field( | ||
| "api_key", | ||
| description=( | ||
| "Authentication mode. 'api_key' uses the api_key in params (default)" |
There was a problem hiding this comment.
Missing separator between the concatenated strings — this renders as …(default)'entra' uses…. Add a period + space at the end of this line: "…(default). ".
| # Copy params so we can modify values (eg api_key) without changing request.params. | ||
| params: dict[str, Any] = dict(request.params) | ||
|
|
||
| if request.auth_mode == "entra": |
There was a problem hiding this comment.
Related to the new explicit auth UI: auth_mode="api_key" here doesn't actually guarantee API-key auth. OpenAITarget.__init__ still silently falls back to get_azure_openai_auth(endpoint) whenever there's no key in params, no env var, and "azure" in endpoint.lower(). Combined with the frontend's if (!isEntra && apiKey) params.api_key = apiKey, a user who picks the API Key radio and leaves the field empty will end up authenticated via Entra without ever asking for it.
Now that we have an explicit auth_mode flag, please make auth_mode="api_key" strictly require a key (in params or env) and raise a clear ValueError here if neither is present. The OpenAI "no key → Entra" fallback was a UX shortcut that made sense before the user could choose; with this PR it actively contradicts the choice.
2 participants
Description
Adding option for new targets to use entra auth instead of API key only.
Screenshots:
(new auth option):
(new test target added successfully!)
(user validation for non-azure endpoints)
Tests and Documentation
Ran existing tests, added new ones (todo)