diff --git a/e2e/tests/agent-network-provider-save-refused.spec.ts b/e2e/tests/agent-network-provider-save-refused.spec.ts new file mode 100644 index 000000000..50b2564b5 --- /dev/null +++ b/e2e/tests/agent-network-provider-save-refused.spec.ts @@ -0,0 +1,195 @@ +/** + * Agent Network provider save: what the operator sees when the backend refuses. + * + * Saving a provider checks its upstream URL and credential against the vendor + * before storing anything, so a 422 naming the field to correct is an ordinary + * outcome of the form rather than a server fault. Four things have to hold, + * and each one has been wrong at some point: + * + * - while the check runs the submit says so and refuses a second click. The + * vendor call is the slow part and an unreachable upstream drags it out to + * a timeout, which looked exactly like a button that had not registered + * the first click. + * - exactly ONE toast, the shared "Request failed with status code N", which + * quotes the API's own sentence. A second toast from the save path said the + * same thing under a vaguer title. + * - the toast is styled as a failure. notify() renders green with a check + * mark unless told otherwise, so a refusal announced itself as a success. + * - the modal stays open with the typed values intact. The API never returns + * an API key, so closing the form loses it with nowhere to retype it. + * + * The refusal is mocked rather than provoked: the backend check ships with a + * management build these tests do not pin, and what is under test here is the + * dashboard's handling of the response, not the vendor call that produces it. + * + * Like the other agent-network specs, this one builds its own context rather + * than taking the shared dashboardAsOwner fixture: the Agent Network menu is + * deployment-gated behind a localStorage override that has to be in place + * before the first navigation, which addInitScript on an own context is the + * way to do. The route interception below is a second reason — the shared page + * is worker-scoped, and a 422 left on it would follow every later test. + */ +import { type Browser, expect, type Page, test } from "@playwright/test"; +import { loginToApp, navigateTo } from "../helpers/auth"; +import { generateRandomName } from "../helpers/utils"; + +const AGENT_NETWORK_CONFIG_KEY = "netbird-test-agent-network"; +const PROVIDERS_ENDPOINT = /\/api\/agent-network\/providers(\?|$)/; +const PROVIDER_PREFIX = "e2e-refused-"; +const TITLE_TESTID = "notification-title"; + +// The message a refused save carries: the backend names which of the two +// fields is at fault, without a status code and without echoing the URL. +const REFUSAL = "the upstream url could not be reached: no such host"; + +// Matched case-insensitively on purpose. The backend lowercases its messages +// (WriteError does, and the copy is written for it) while the toast uppercases +// the first character before rendering. Asserting either spelling would pin +// the test to that transform rather than to the sentence the operator reads. +const REFUSAL_TEXT = new RegExp( + REFUSAL.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"), + "i", +); + +async function newAgentNetworkPage(browser: Browser): Promise<{ + page: Page; + close: () => Promise; +}> { + const context = await browser.newContext({ + storageState: "e2e/fixtures/auth/owner.json", + }); + await context.addInitScript( + ([key, value]) => { + try { + window.localStorage.setItem(key as string, value as string); + } catch (e) {} + }, + [AGENT_NETWORK_CONFIG_KEY, "enabled"], + ); + const page = await context.newPage(); + await loginToApp(page, "owner"); + return { page, close: () => context.close() }; +} + +// How long the mocked create takes to answer. The real check is a vendor round +// trip, so a save is never instant; holding the response gives the in-flight +// assertions a window to run in without racing the toast. +const REFUSAL_DELAY_MS = 1500; + +// refuseProviderCreate answers the create with the 422 the credential check +// produces. Only POST is intercepted: the page still lists providers, and the +// settings bootstrap that may precede the create is left alone. +async function refuseProviderCreate(page: Page) { + await page.route(PROVIDERS_ENDPOINT, async (route) => { + if (route.request().method() !== "POST") return route.continue(); + const origin = route.request().headers()["origin"] || "*"; + await new Promise((resolve) => setTimeout(resolve, REFUSAL_DELAY_MS)); + await route.fulfill({ + status: 422, + headers: { + "content-type": "application/json", + "access-control-allow-origin": origin, + }, + body: JSON.stringify({ code: 422, message: REFUSAL }), + }); + }); +} + +test.describe + .serial("Agent Network refused provider save @agent-network", () => { + test("reports the refusal once and keeps the form open", async ({ + browser, + }) => { + const { page, close } = await newAgentNetworkPage(browser); + try { + await refuseProviderCreate(page); + + // navigateTo rather than goto: it dismisses the setup modal that greets a + // fresh account and clears the scroll lock it leaves behind, either of + // which swallows the clicks below. + await navigateTo(page, "/agent-network/providers"); + + await page + .getByRole("button", { name: "Connect Provider" }) + .first() + .click({ force: true }); + + const providerName = generateRandomName(PROVIDER_PREFIX); + await page + .getByTestId("agent-network-provider-name-input") + .fill(providerName); + await page + .getByTestId("agent-network-provider-key-input") + .fill("sk-e2e-refused-key"); + + // The submit lives on the Models tab — the Provider tab's primary button + // only advances to it. + await page.getByRole("tab", { name: "Models" }).click({ force: true }); + const submit = page.getByTestId("agent-network-provider-submit"); + await expect(submit).toBeEnabled(); + + const refusal = page.waitForResponse( + (resp) => + PROVIDERS_ENDPOINT.test(resp.url()) && + resp.request().method() === "POST", + { timeout: 30_000 }, + ); + await submit.click({ force: true }); + + // ---- the wait is visible while it lasts ---- + // A second create is not idempotent, so the button has to say it is + // working and stop taking clicks rather than sit there looking untouched. + // Asserted before the response is awaited: the mock holds it open for + // exactly this window. + await expect(submit).toContainText("Connecting provider"); + await expect(submit).toBeDisabled(); + + expect((await refusal).status()).toBe(422); + + // ---- the toast says what the API said ---- + const title = page.getByTestId(TITLE_TESTID).first(); + await expect(title).toContainText("Request failed with status code 422"); + await expect( + page.locator("[data-toast-notification]").first(), + ).toContainText(REFUSAL_TEXT); + + // ---- and only that toast ---- + // The save path used to add its own on top, so the count is the + // assertion rather than the presence of the right one. + await expect(page.locator("[data-toast-notification]")).toHaveCount(1); + await expect(page.getByText("Failed to connect provider")).toHaveCount(0); + + // ---- styled as a failure, not a success ---- + // notify() paints the icon tile green with a check unless the caller + // says otherwise, which is how a refusal once looked like a success. + // + // A failure reaches the tile two ways — notify()'s own error state, and + // a caller passing its own colour and icon, which is what the shared + // request-failed toast does — so what has to hold is the negative: this + // is not the default success tile. + const icon = page.getByTestId("notification-icon"); + await expect(icon).toBeVisible(); + await expect(icon).not.toHaveAttribute("data-variant", "success"); + + // ---- the form is still there, still holding what was typed ---- + // The submit only exists while the modal is open, so its presence is the + // check that nothing closed underneath the toast. It also has to come + // back out of the in-flight state, or the retry the toast asks for is + // impossible. + await expect(submit).toBeVisible(); + await expect(submit).toBeEnabled(); + await expect(submit).toContainText("Connect Provider"); + await page.getByRole("tab", { name: "Provider" }).click({ force: true }); + await expect( + page.getByTestId("agent-network-provider-name-input"), + ).toHaveValue(providerName); + // The key matters most: the API never returns one, so a form that lost + // it leaves the operator with nothing to correct. + await expect( + page.getByTestId("agent-network-provider-key-input"), + ).toHaveValue("sk-e2e-refused-key"); + } finally { + await close(); + } + }); +}); diff --git a/src/components/Notification.tsx b/src/components/Notification.tsx index a309e99d9..b36541c77 100644 --- a/src/components/Notification.tsx +++ b/src/components/Notification.tsx @@ -180,6 +180,22 @@ export default function Notification({ >
("provider"); + // A save reaches the vendor to check the url and credential before storing + // anything, so it holds for as long as that round trip takes — up to a + // timeout. Nothing else on the footer moves while it does. + const [saveInFlight, setSaveInFlight] = useState(false); const [providerId, setProviderId] = useState( provider?.providerId ?? "openai_api", ); @@ -479,45 +484,56 @@ export default function AIProviderModal({ identityHeaderGroups: identityHeaderGroups.trim(), } : {}; - if (isEdit && provider) { - await updateProvider(provider.id, { + setSaveInFlight(true); + try { + if (isEdit && provider) { + const saved = await updateProvider(provider.id, { + providerId, + name, + upstreamUrl, + models: submittedModels, + extraValues: sanitizedExtraValues, + ...identityOverrides, + skipTlsVerification: isCustomKind ? skipTlsVerification : false, + metadataDisabled, + // Only forward the API key when the user actually rotated it + ...(apiKey && apiKey.trim() !== MASKED_API_KEY ? { apiKey } : {}), + }); + // The url and credential are checked against the vendor before the + // change is stored, so a save can be refused for a reason the operator + // has to fix here. Closing would throw away the key they just typed — + // and it never comes back from the API to be typed over again. + if (!saved) return; + handleClose(); + return; + } + // First create: bootstrap the account's endpoint before the provider + // exists, as an explicit settings POST. A failure keeps the wizard open + // (the provider isn't created either) so the operator sees the error and + // can retry — this used to be a silent backend side effect. + if (!settingsBootstrapped) { + const bootstrapped = await bootstrapAgentNetworkSettings( + bootstrapCluster.trim(), + ); + if (!bootstrapped) return; + } + const created = await addProvider({ providerId, name, upstreamUrl, - models: submittedModels, + apiKey, extraValues: sanitizedExtraValues, ...identityOverrides, skipTlsVerification: isCustomKind ? skipTlsVerification : false, metadataDisabled, - // Only forward the API key when the user actually rotated it - ...(apiKey && apiKey.trim() !== MASKED_API_KEY ? { apiKey } : {}), + models: submittedModels, + enabled: true, }); + if (!created) return; handleClose(); - return; + } finally { + setSaveInFlight(false); } - // First create: bootstrap the account's endpoint before the provider - // exists, as an explicit settings POST. A failure keeps the wizard open - // (the provider isn't created either) so the operator sees the error and - // can retry — this used to be a silent backend side effect. - if (!settingsBootstrapped) { - const bootstrapped = await bootstrapAgentNetworkSettings( - bootstrapCluster.trim(), - ); - if (!bootstrapped) return; - } - await addProvider({ - providerId, - name, - upstreamUrl, - apiKey, - extraValues: sanitizedExtraValues, - ...identityOverrides, - skipTlsVerification: isCustomKind ? skipTlsVerification : false, - metadataDisabled, - models: submittedModels, - enabled: true, - }); - handleClose(); }; // providerOptions are sorted into three groups, first-party AI Providers @@ -600,27 +616,24 @@ export default function AIProviderModal({ // provider has to supply the key the operator is typing. // // That reuse is only right while the form still describes the record the - // credential belongs to. The API resolves a provider_id request entirely - // from the stored row — vendor, upstream and key — so switching the vendor - // dropdown and then asking by record id answers with the OLD vendor's models - // and offers them for the new one. A replacement key typed over the mask is - // the same mistake in the other direction: the operator wants that key - // tested, not the one already saved. + // credential belongs to. The API takes the vendor from the stored row, so + // switching the vendor dropdown and then asking by record id answers with + // the OLD vendor's models and offers them for the new one. A replacement key + // typed over the mask is the same mistake in the other direction: the + // operator wants that key tested, not the one already saved. // - // Changing the upstream URL invalidates the saved path: discovery sends - // provider_id and the API resolves the URL from the stored row, so a changed - // URL would silently test the old endpoint. Require a freshly entered key - // when the URL differs; canDiscoverModels will block discovery until the - // operator provides one. + // A retyped URL is neither. It is sent with the record id and overrides the + // stored upstream, so the endpoint on the form is the one listed against — + // asking for the key back would be asking for something the API never + // returned. const useSavedCredential = isEdit && !!provider?.id && providerId === provider.providerId && - upstreamUrl === provider.upstreamUrl && apiKey.trim() === MASKED_API_KEY; const canDiscoverModels = useMemo(() => { - if (useSavedCredential) return true; + if (useSavedCredential) return upstreamUrl.trim() !== ""; return ( upstreamUrl.trim() !== "" && apiKey.trim() !== "" && @@ -631,9 +644,13 @@ export default function AIProviderModal({ }, [useSavedCredential, upstreamUrl, apiKey]); const loadModelsFromProvider = async () => { - const found = await discovered.discover( + await discovered.discover( useSavedCredential && provider?.id - ? { catalog_provider_id: providerId, provider_id: provider.id } + ? { + catalog_provider_id: providerId, + provider_id: provider.id, + upstream_url: upstreamUrl.trim(), + } : { catalog_provider_id: providerId, upstream_url: upstreamUrl.trim(), @@ -642,6 +659,28 @@ export default function AIProviderModal({ ); }; + // Named rather than used inline: it gates the Load button, the Save button + // and the result line, so one definition keeps them from drifting apart. + const discoveryInFlight = discovered.isLoading; + + // The footer renders the same submit twice — once on the models tab and once + // on the mappings tab — so its state is defined once here. + const submitDisabled = + !canContinueFromProvider || discoveryInFlight || saveInFlight; + const submitLabel = saveInFlight ? ( + <> + + {isEdit ? "Saving changes…" : "Connecting provider…"} + + ) : isEdit ? ( + "Save Changes" + ) : ( + <> + + Connect Provider + + ); + // A discovery result describes one provider, endpoint and credential. Once // any of those changes on screen, the previous answer is about a // configuration that is no longer being edited, so it is dropped rather than @@ -932,6 +971,7 @@ export default function AIProviderModal({ helpText={"The API key issued by the provider."} > setName(e.target.value)} placeholder={"e.g. OpenAI"} @@ -1419,17 +1460,23 @@ export default function AIProviderModal({ {!canDiscoverModels && ( - Enter the endpoint URL and API key first. + {useSavedCredential + ? "Enter the endpoint URL first." + : "Enter the endpoint URL and API key first."} )} {discovered.notSupported && ( @@ -1447,7 +1494,7 @@ export default function AIProviderModal({ )}
- {!discovered.isLoading && + {!discoveryInFlight && !discovered.error && discovered.models.length > 0 && ( @@ -1580,17 +1627,11 @@ export default function AIProviderModal({ ) : ( )} @@ -1602,17 +1643,11 @@ export default function AIProviderModal({ )} diff --git a/src/modules/agent-network/AIProvidersProvider.tsx b/src/modules/agent-network/AIProvidersProvider.tsx index 68fdd14f2..ce0cdb492 100644 --- a/src/modules/agent-network/AIProvidersProvider.tsx +++ b/src/modules/agent-network/AIProvidersProvider.tsx @@ -1,6 +1,7 @@ "use client"; import { notify } from "@components/Notification"; +import { IconCircleX } from "@tabler/icons-react"; import useFetchApi, { useApiCall } from "@utils/api"; import React, { createContext, @@ -215,6 +216,18 @@ function fromAPI(p: APIProvider): AIProvider { }; } +// notify() renders green with a check mark unless it is told otherwise: its red +// styling comes from the promise path, and none of these use it. A failure that +// looks like a success is worse than saying nothing, so every failure toast in +// this file goes through here. +function notifyFailure(props: { title: string; description: string }) { + return notify({ + ...props, + backgroundColor: "bg-red-500", + icon: , + }); +} + function toAPIModels(models: ProviderModel[]): APIProviderModel[] { // undefined cache rates stay undefined so JSON.stringify omits the // key: an omitted rate inherits NetBird's default, an explicit 0 @@ -495,7 +508,13 @@ type AIProvidersContextValue = { closeWizard: () => void; isWizardOpen: boolean; addProvider: (input: ProviderConnectInput) => Promise; - updateProvider: (id: string, updates: ProviderUpdateInput) => Promise; + // Resolves false when the save was refused — the backend checks a provider's + // url and credential before storing them, so a rejected edit must leave the + // form open with what the operator typed still in it. + updateProvider: ( + id: string, + updates: ProviderUpdateInput, + ) => Promise; toggleProvider: (id: string) => Promise; deleteProvider: (id: string) => Promise; addPolicy: ( @@ -596,6 +615,11 @@ export default function AIProvidersProvider({ children }: Readonly) { true, agentNetworkEnabled, ); + // Default error handling on purpose: a failed save raises the shared + // "Request failed with status code N" toast, which carries the message the + // API sent — for a refused provider that is the sentence naming the url or + // the credential. The save paths below stay silent on failure rather than + // adding a second toast that says the same thing in different words. const providersApi = useApiCall("/agent-network/providers"); const { data: apiPolicies, mutate: mutatePolicies } = useFetchApi< @@ -665,21 +689,24 @@ export default function AIProvidersProvider({ children }: Readonly) { const addProvider = useCallback( async (input: ProviderConnectInput) => { + let created: APIProvider; try { - const created = await providersApi.post(toCreateRequest(input)); - await mutate(); - notify({ - title: "AI provider connected", - description: `${created.name} is now available on your agent network endpoint.`, - }); - return fromAPI(created); - } catch (err) { - notify({ - title: "Failed to connect provider", - description: err instanceof Error ? err.message : String(err), - }); + created = await providersApi.post(toCreateRequest(input)); + } catch { + // Reported already by the shared request-failed toast. Returning + // undefined is what keeps the modal open on the fields to correct. return undefined; } + // Outside the catch: the provider exists from here on, and a failed + // revalidation is a stale list rather than a failed create. Reporting it + // as one would hold the modal open on a form whose next submit creates a + // second provider. + await mutate().catch(() => undefined); + notify({ + title: "AI provider connected", + description: `${created.name} is now available on your agent network endpoint.`, + }); + return fromAPI(created); }, [providersApi, mutate], ); @@ -687,7 +714,16 @@ export default function AIProvidersProvider({ children }: Readonly) { const updateProvider = useCallback( async (id: string, updates: ProviderUpdateInput) => { const existing = (apiProviders ?? []).find((p) => p.id === id); - if (!existing) return; + if (!existing) { + // The update merges onto the record as this browser last saw it, so a + // provider deleted elsewhere leaves nothing to merge onto. Silence + // here read as a save that did nothing. + notifyFailure({ + title: "Provider not updated", + description: "This provider is no longer available. Reload the page.", + }); + return false; + } const merged: APIProviderRequest = { provider_id: updates.providerId ?? existing.provider_id, name: updates.name ?? existing.name, @@ -714,17 +750,18 @@ export default function AIProvidersProvider({ children }: Readonly) { }; try { await providersApi.put(merged, `/${id}`); - await mutate(); - notify({ - title: "Provider updated", - description: "Settings saved.", - }); - } catch (err) { - notify({ - title: "Failed to update provider", - description: err instanceof Error ? err.message : String(err), - }); + } catch { + // Reported already by the shared request-failed toast. + return false; } + // See addProvider: a failed revalidation is a stale list, not a failed + // write, and must not send the operator back to resubmit one. + await mutate().catch(() => undefined); + notify({ + title: "Provider updated", + description: "Settings saved.", + }); + return true; }, [apiProviders, providersApi, mutate], ); @@ -748,7 +785,7 @@ export default function AIProvidersProvider({ children }: Readonly) { description: "Endpoint will be torn down on next mapping update.", }); } catch (err) { - notify({ + notifyFailure({ title: "Failed to remove provider", description: err instanceof Error ? err.message : String(err), }); @@ -768,7 +805,7 @@ export default function AIProvidersProvider({ children }: Readonly) { }); return policyFromAPI(created); } catch (err) { - notify({ + notifyFailure({ title: "Failed to create policy", description: err instanceof Error ? err.message : String(err), }); @@ -804,7 +841,7 @@ export default function AIProvidersProvider({ children }: Readonly) { description: "Settings saved.", }); } catch (err) { - notify({ + notifyFailure({ title: "Failed to update policy", description: err instanceof Error ? err.message : String(err), }); @@ -832,7 +869,7 @@ export default function AIProvidersProvider({ children }: Readonly) { description: "Policy deleted.", }); } catch (err) { - notify({ + notifyFailure({ title: "Failed to remove policy", description: err instanceof Error ? err.message : String(err), }); @@ -852,7 +889,7 @@ export default function AIProvidersProvider({ children }: Readonly) { }); return guardrailFromAPI(created); } catch (err) { - notify({ + notifyFailure({ title: "Failed to create guardrail", description: err instanceof Error ? err.message : String(err), }); @@ -881,7 +918,7 @@ export default function AIProvidersProvider({ children }: Readonly) { description: "Settings saved.", }); } catch (err) { - notify({ + notifyFailure({ title: "Failed to update guardrail", description: err instanceof Error ? err.message : String(err), }); @@ -901,7 +938,7 @@ export default function AIProvidersProvider({ children }: Readonly) { "Existing policies still reference this guardrail until you detach it.", }); } catch (err) { - notify({ + notifyFailure({ title: "Failed to remove guardrail", description: err instanceof Error ? err.message : String(err), }); @@ -921,7 +958,7 @@ export default function AIProvidersProvider({ children }: Readonly) { }); return budgetRuleFromAPI(created); } catch (err) { - notify({ + notifyFailure({ title: "Failed to create global limit", description: err instanceof Error ? err.message : String(err), }); @@ -955,7 +992,7 @@ export default function AIProvidersProvider({ children }: Readonly) { description: "Settings saved.", }); } catch (err) { - notify({ + notifyFailure({ title: "Failed to update global limit", description: err instanceof Error ? err.message : String(err), }); @@ -983,7 +1020,7 @@ export default function AIProvidersProvider({ children }: Readonly) { description: "Global limit deleted.", }); } catch (err) { - notify({ + notifyFailure({ title: "Failed to remove global limit", description: err instanceof Error ? err.message : String(err), }); @@ -1002,7 +1039,7 @@ export default function AIProvidersProvider({ children }: Readonly) { } catch (err) { const code = (err as { code?: number })?.code; if (code !== 409) { - notify({ + notifyFailure({ title: "Failed to set up the agent network endpoint", description: err instanceof Error ? err.message : String(err), }); @@ -1024,7 +1061,7 @@ export default function AIProvidersProvider({ children }: Readonly) { // row there is nothing to echo — and no row to update; the backend // would 404 the PUT anyway. if (!settings) { - notify({ + notifyFailure({ title: "Failed to update account controls", description: "Agent Network has not been set up yet.", }); @@ -1039,7 +1076,7 @@ export default function AIProvidersProvider({ children }: Readonly) { }); return true; } catch (err) { - notify({ + notifyFailure({ title: "Failed to update account controls", description: err instanceof Error ? err.message : String(err), }); diff --git a/src/modules/agent-network/useDiscoveredModels.ts b/src/modules/agent-network/useDiscoveredModels.ts index f2abe34de..d58d8bef7 100644 --- a/src/modules/agent-network/useDiscoveredModels.ts +++ b/src/modules/agent-network/useDiscoveredModels.ts @@ -27,6 +27,8 @@ type DiscoveryResponse = { models: DiscoveredModel[] }; type DiscoveryRequest = { catalog_provider_id: string; + // Sent alongside provider_id, this overrides the record's stored upstream, + // which is how a retyped URL is listed against before it is saved. upstream_url?: string; // Exactly one of these. api_key is for a provider being typed in and not yet // saved; provider_id reuses a saved record's stored credential, which is how