Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion backend/onyx/server/manage/image_generation/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -374,14 +374,26 @@ def update_config(
old_provider.name = f"{old_provider.name}-old-{old_llm_provider_id}"
db_session.flush()

# Determine actual API key to use:
# - Clone mode (source_llm_provider_id): API key comes from source provider
# - New credentials mode: Use provided api_key, or preserve existing if not changed
actual_api_key = config_update.api_key
if (
config_update.source_llm_provider_id is None
and not config_update.api_key_changed
and old_provider
):
# Preserve existing API key when not explicitly changed
actual_api_key = old_provider.api_key

# 3. Build and create new LLM provider
provider_request = _build_llm_provider_request(
db_session=db_session,
image_provider_id=image_provider_id,
model_name=config_update.model_name,
source_llm_provider_id=config_update.source_llm_provider_id,
provider=config_update.provider,
api_key=config_update.api_key,
api_key=actual_api_key,
api_base=config_update.api_base,
api_version=config_update.api_version,
deployment_name=config_update.deployment_name,
Expand Down
19 changes: 17 additions & 2 deletions backend/onyx/server/manage/image_generation/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,15 @@
from onyx.db.models import ImageGenerationConfig as ImageGenerationConfigModel


def _mask_api_key(api_key: str | None) -> str | None:
"""Mask API key, showing first 4 and last 4 characters."""
if not api_key:
return None
if len(api_key) <= 8:
return "****"
return api_key[:4] + "****" + api_key[-4:]


class TestImageGenerationRequest(BaseModel):
"""Request model for testing image generation API key.

Expand Down Expand Up @@ -79,6 +88,9 @@ class ImageGenerationConfigUpdate(BaseModel):
api_version: str | None = None
deployment_name: str | None = None

# If False and using new credentials mode, preserve existing API key from DB
api_key_changed: bool = False


class ImageGenerationConfigView(BaseModel):
"""Response model for image generation config with related data."""
Expand Down Expand Up @@ -117,10 +129,13 @@ class ImageGenerationCredentials(BaseModel):
def from_model(
cls, config: "ImageGenerationConfigModel"
) -> "ImageGenerationCredentials":
"""Convert database model to credentials model."""
"""Convert database model to credentials model.

Note: API key is masked for security - only first 4 and last 4 chars shown.
"""
llm_provider = config.model_configuration.llm_provider
return cls(
api_key=llm_provider.api_key,
api_key=_mask_api_key(llm_provider.api_key),
api_base=llm_provider.api_base,
api_version=llm_provider.api_version,
deployment_name=llm_provider.deployment_name,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,13 @@ export function ImageGenFormWrapper<T extends FormValues>({
const isEditMode = !!existingConfig;

// Compute API key options from existing providers matching this image provider
// API keys from LLM providers are already masked by backend (first 4 + **** + last 4)
const apiKeyOptions = useMemo(() => {
return existingProviders
.filter((p) => p.provider === imageProvider.provider_name)
.map((provider) => ({
value: `existing:${provider.id}:${provider.name}`,
label: `${provider.api_key || "****"}`,
label: provider.api_key || "****",
}));
}, [existingProviders, imageProvider.provider_name]);

Expand Down Expand Up @@ -181,20 +182,27 @@ export function ImageGenFormWrapper<T extends FormValues>({
});
}
} else {
// New credentials mode - test the new API key first
const result = await testImageGenerationApiKey(payload.modelName, {
provider: payload.provider,
apiKey: payload.apiKey,
apiBase: payload.apiBase,
apiVersion: payload.apiVersion,
deploymentName: payload.deploymentName,
});
// New credentials mode - check if API key was changed from initial (masked) value
const initialApiKey = (mergedInitialValues as Record<string, unknown>)
.api_key as string | undefined;
const apiKeyChanged = apiKeyValue !== initialApiKey;

if (!result.ok) {
setApiStatus("error");
setErrorMessage(result.errorMessage || "API key validation failed");
setIsSubmitting(false);
return;
// Test the API key first (only if changed or creating new config)
if (apiKeyChanged) {
const result = await testImageGenerationApiKey(payload.modelName, {
provider: payload.provider,
apiKey: payload.apiKey,
apiBase: payload.apiBase,
apiVersion: payload.apiVersion,
deploymentName: payload.deploymentName,
});

if (!result.ok) {
setApiStatus("error");
setErrorMessage(result.errorMessage || "API key validation failed");
setIsSubmitting(false);
return;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: In edit mode with a masked API key (e.g., "key1key4"), comparing the form's current value to initialApiKey will incorrectly detect a change when the user hasn't modified the field. This causes unnecessary API testing of a masked key, which will fail. The logic should explicitly check if the value looks like a masked key (contains "") to properly detect actual user changes vs. the initial masked value.

Prompt To Fix With AI
This is a comment left during a code review.
Path: web/src/app/admin/configuration/image-generation/forms/ImageGenFormWrapper.tsx
Line: 185:206

Comment:
**logic:** In edit mode with a masked API key (e.g., "key1****key4"), comparing the form's current value to `initialApiKey` will incorrectly detect a change when the user hasn't modified the field. This causes unnecessary API testing of a masked key, which will fail. The logic should explicitly check if the value looks like a masked key (contains "****") to properly detect actual user changes vs. the initial masked value.

How can I resolve this? If you propose a fix, please make it concise.


// Create or update config
Expand All @@ -206,6 +214,7 @@ export function ImageGenFormWrapper<T extends FormValues>({
apiBase: payload.apiBase,
apiVersion: payload.apiVersion,
deploymentName: payload.deploymentName,
apiKeyChanged,
});
} else {
await createImageGenerationConfig({
Expand Down
5 changes: 5 additions & 0 deletions web/src/lib/configuration/imageConfigurationService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,9 @@ export interface ImageGenerationConfigUpdateOptions {
apiBase?: string;
apiVersion?: string;
deploymentName?: string;

// If true, apiKey was changed by user; if false, backend preserves existing key
apiKeyChanged?: boolean;
}

/**
Expand All @@ -197,6 +200,8 @@ export async function updateImageGenerationConfig(
api_base: options.apiBase,
api_version: options.apiVersion,
deployment_name: options.deploymentName,
// If false, backend preserves existing API key
api_key_changed: options.apiKeyChanged ?? false,
}),
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,8 @@ export const OptionsList: React.FC<OptionsListProps> = ({
{/* Matched/Filtered Options */}
{matchedOptions.map((option, idx) => {
const globalIndex = idx + indexOffset;
const isExact = isExactMatch(option);
// Only highlight first exact match, not all matches
const isExact = idx === 0 && isExactMatch(option);
return (
<OptionItem
key={option.value}
Expand Down
Loading