From d667d68e3ce489c50a3c34eda46faa8c55431c0a Mon Sep 17 00:00:00 2001 From: Ethan <39577870+ethanndickson@users.noreply.github.com> Date: Tue, 25 Aug 2026 11:25:16 +0100 Subject: [PATCH] fix(site): harden organization-scoped model loading (#28495) Previously, the new agent form treated the dashboard organization as a usable resource scope while permission filtering was still pending. This could request a provisional empty model catalog, briefly show the no-model setup notice, and lose the selected organization when the form remounted. A malformed organization models response could also crash the Models settings page. Wait for authoritative permitted organizations before enabling organization-scoped resources, persist valid organization selections across remounts, and reject malformed model responses while retaining a defensive rendering guard. ## UAT An adversarial [Coder Agents UAT run](https://dev.coder.com/agents/980a5a6d-e91b-47f0-902e-25d12ac46935) validated this commit on a licensed local deployment with two organizations, a real Anthropic model, and a non-admin user. Under throttled and artificially delayed authorization, no organization-scoped request fired before permissions settled and the false setup notice never appeared, while an authorized zero-model organization still showed the genuine notice. Persistence across reloads, tampered or stale localStorage IDs, membership revocation, attachment-preserving organization switches, single-organization deployments, and induced malformed or failed model responses on the Models settings page all behaved correctly. Verdict: PASS. --- site/src/api/api.test.ts | 12 ++ site/src/api/api.ts | 3 + .../OrganizationModelsLayout.stories.tsx | 18 +++ .../ModelsPage/organizationModels.tsx | 2 +- .../components/AgentCreateForm.stories.tsx | 133 ++++++++++++++++-- .../AgentsPage/components/AgentCreateForm.tsx | 41 ++++-- 6 files changed, 185 insertions(+), 24 deletions(-) diff --git a/site/src/api/api.test.ts b/site/src/api/api.test.ts index b4e6413360e..d36eec59f0c 100644 --- a/site/src/api/api.test.ts +++ b/site/src/api/api.test.ts @@ -451,6 +451,18 @@ describe("api.ts", () => { expect(result).toStrictEqual(responseData); }); + it("rejects chat model responses without a models array", async () => { + vi.spyOn(axiosInstance, "get").mockResolvedValueOnce({ + data: { providers: [], unsupported_providers: [] }, + }); + + await expect( + API.experimental.getChatModels(organizationId), + ).rejects.toThrow( + "Invalid chat models response: models must be an array.", + ); + }); + it.each<[string, () => Promise]>([ [ "/api/experimental/organizations/organization%2Fid/chats/models", diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 1fe4ca502e4..03b2a0e7c63 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -3903,6 +3903,9 @@ class ExperimentalApiMethods { await this.axios.get( chatModelsPath(organizationId), ); + if (!Array.isArray(response.data?.models)) { + throw new Error("Invalid chat models response: models must be an array."); + } return response.data; }; diff --git a/site/src/pages/AISettingsPage/ModelsPage/OrganizationModelsLayout.stories.tsx b/site/src/pages/AISettingsPage/ModelsPage/OrganizationModelsLayout.stories.tsx index a0ebc71eef9..d0c8c56a559 100644 --- a/site/src/pages/AISettingsPage/ModelsPage/OrganizationModelsLayout.stories.tsx +++ b/site/src/pages/AISettingsPage/ModelsPage/OrganizationModelsLayout.stories.tsx @@ -358,6 +358,24 @@ export const PermissionLoadErrorShowsAlert: Story = { }, }; +export const ModelLoadErrorShowsAlert: Story = { + beforeEach: () => { + spyOn(API.experimental, "getChatModels").mockRejectedValue( + new Error("Invalid chat models response: models must be an array."), + ); + spyOn(API, "checkAuthorization").mockResolvedValue({}); + }, + parameters: { queries: [] }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + expect( + await canvas.findByText( + "Invalid chat models response: models must be an array.", + ), + ).toBeVisible(); + }, +}; + export const Loading: Story = { beforeEach: () => { spyOn(API.experimental, "getChatModels").mockImplementation( diff --git a/site/src/pages/AISettingsPage/ModelsPage/organizationModels.tsx b/site/src/pages/AISettingsPage/ModelsPage/organizationModels.tsx index 41d69864d07..8aeb15fdbbc 100644 --- a/site/src/pages/AISettingsPage/ModelsPage/organizationModels.tsx +++ b/site/src/pages/AISettingsPage/ModelsPage/organizationModels.tsx @@ -29,7 +29,7 @@ export const useAccessibleModelOrganizations = ( ); const accessibleOrganizations = organizations.filter( (organization, index) => - (queries[index]?.data?.models.length ?? 0) > 0 || + (queries[index]?.data?.models?.length ?? 0) > 0 || canAccessOrganizationChatModelConfig( permissionsQuery.data?.[organization.id], ), diff --git a/site/src/pages/AgentsPage/components/AgentCreateForm.stories.tsx b/site/src/pages/AgentsPage/components/AgentCreateForm.stories.tsx index 7223aaee243..b47bf6dfdda 100644 --- a/site/src/pages/AgentsPage/components/AgentCreateForm.stories.tsx +++ b/site/src/pages/AgentsPage/components/AgentCreateForm.stories.tsx @@ -1,6 +1,6 @@ import type { Decorator, Meta, StoryObj } from "@storybook/react-vite"; import { delay } from "msw"; -import { useEffect, useState } from "react"; +import { type ComponentProps, useEffect, useState } from "react"; import { QueryClient, QueryClientProvider, useQueryClient } from "react-query"; import { expect, @@ -26,6 +26,7 @@ import { MockChatModel, MockChatModelProviderDescriptor, } from "#/testHelpers/chatModels"; +import { createDeferred, type Deferred } from "#/testHelpers/deferred"; import { MockDefaultOrganization, MockOrganization2, @@ -37,8 +38,15 @@ import { getReasoningEffortForModel, saveReasoningEffortForModel, } from "../utils/reasoningEffort"; -import { AgentCreateForm, emptyInputStorageKey } from "./AgentCreateForm"; +import { + AgentCreateForm, + emptyInputStorageKey, + selectedOrganizationIdStorageKey, +} from "./AgentCreateForm"; +let pendingOrganizationAuthorization: Deferred< + Awaited> +>; let capturedQueryClient: QueryClient | undefined; const permittedOrgsKey = permittedOrganizationsKey({ @@ -298,6 +306,20 @@ type Story = StoryObj; const defaultArgs = meta.args; +const RemountAgentCreateForm = ( + props: ComponentProps, +) => { + const [key, setKey] = useState(0); + return ( + <> + + + + ); +}; + const mockPermittedOrganizations = ( permissions: Record, delayMs = 0, @@ -1644,6 +1666,7 @@ export const OrganizationAuthorizationFailure: Story = { parameters: { showOrganizations: true, organizations: [MockDefaultOrganization, MockOrganization2], + queries: [], }, beforeEach: () => { localStorage.clear(); @@ -1655,10 +1678,18 @@ export const OrganizationAuthorizationFailure: Story = { spyOn(API, "checkAuthorization").mockRejectedValue( new Error("authorization check failed"), ); + spyOn(API.experimental, "getChatModels").mockResolvedValue( + defaultModelCatalog, + ); }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); await canvas.findAllByText(/authorization check failed/i); + expect(API.experimental.getChatModels).not.toHaveBeenCalled(); + expect(API.experimental.getMCPServerConfigs).not.toHaveBeenCalled(); + expect( + API.experimental.getUserChatPersonalModelOverrides, + ).not.toHaveBeenCalled(); expect(canvas.getByRole("button", { name: "Send" })).toBeDisabled(); }, }; @@ -1695,15 +1726,23 @@ export const DelayedOrganizationAuthorization: Story = { parameters: { showOrganizations: true, organizations: [MockDefaultOrganization, MockOrganization2], + queries: [], }, beforeEach: () => { localStorage.setItem(emptyInputStorageKey, "draft message"); - mockPermittedOrganizations( - { - [MockDefaultOrganization.id]: true, - [MockOrganization2.id]: true, - }, - 1_500, + pendingOrganizationAuthorization = createDeferred(); + spyOn(API, "getOrganizations").mockResolvedValue([ + MockDefaultOrganization, + MockOrganization2, + ]); + spyOn(API, "checkAuthorization").mockImplementation( + () => pendingOrganizationAuthorization.promise, + ); + spyOn(API.experimental, "getChatModels").mockImplementation( + async (organizationId) => + organizationId === MockOrganization2.id + ? organization2LocalCatalog + : { ...defaultModelCatalog, models: [] }, ); }, args: { @@ -1724,6 +1763,15 @@ export const DelayedOrganizationAuthorization: Story = { await expect( canvas.getByRole("button", { name: "More options" }), ).toBeDisabled(); + expect(API.experimental.getChatModels).not.toHaveBeenCalled(); + expect(API.experimental.getMCPServerConfigs).not.toHaveBeenCalled(); + expect( + API.experimental.getUserChatPersonalModelOverrides, + ).not.toHaveBeenCalled(); + expect( + canvas.queryByText(/AI models aren't available yet/i), + ).not.toBeInTheDocument(); + expect(canvas.queryByText("No model is available")).not.toBeInTheDocument(); // dispatchEvent returns false when a handler accepted the drop // via preventDefault, giving a race-free accepted/ignored signal. const dropFile = (name: string): boolean => { @@ -1741,12 +1789,28 @@ export const DelayedOrganizationAuthorization: Story = { // must be ignored. expect(dropFile("drop.txt")).toBe(true); expect(canvas.queryByLabelText("Remove drop.txt")).not.toBeInTheDocument(); - // The pending option list is unfiltered, so the picker must stay hidden. expect( canvas.queryByRole("button", { name: /organization/i }), ).not.toBeInTheDocument(); - await waitFor(() => expect(sendButton).toBeEnabled(), { timeout: 3_000 }); - await canvas.findByRole("button", { name: /organization/i }); + + pendingOrganizationAuthorization.resolve({ + [MockDefaultOrganization.id]: false, + [MockOrganization2.id]: true, + }); + + await waitFor(() => expect(sendButton).toBeEnabled()); + expect(API.experimental.getChatModels).toHaveBeenCalledWith( + MockOrganization2.id, + ); + expect(API.experimental.getChatModels).not.toHaveBeenCalledWith( + MockDefaultOrganization.id, + ); + expect(API.experimental.getMCPServerConfigs).toHaveBeenCalledWith( + MockOrganization2.id, + ); + expect( + API.experimental.getUserChatPersonalModelOverrides, + ).toHaveBeenCalledWith(MockOrganization2.id, "me"); // Positive control: once settled the same drop is accepted and // attaches, so the pending-state assertions exercised a real path. expect(dropFile("after.txt")).toBe(false); @@ -1756,6 +1820,53 @@ export const DelayedOrganizationAuthorization: Story = { }, }; +export const SelectedOrganizationSurvivesRemount: Story = { + parameters: { + showOrganizations: true, + organizations: [MockDefaultOrganization, MockOrganization2], + queries: [], + }, + args: { ...defaultArgs }, + render: (args) => , + beforeEach: () => { + mockPermittedOrganizations({ + [MockDefaultOrganization.id]: true, + [MockOrganization2.id]: true, + }); + spyOn(API.experimental, "getChatModels").mockImplementation( + async (organizationId) => + organizationId === MockOrganization2.id + ? organization2LocalCatalog + : defaultModelCatalog, + ); + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await userEvent.click( + await canvas.findByRole("button", { + name: `Organization: ${MockDefaultOrganization.display_name}`, + }), + ); + await userEvent.click( + await screen.findByRole("option", { + name: MockOrganization2.display_name, + }), + ); + await waitFor(() => { + expect(localStorage.getItem(selectedOrganizationIdStorageKey)).toBe( + MockOrganization2.id, + ); + }); + + await userEvent.click(canvas.getByRole("button", { name: "Remount form" })); + expect( + await canvas.findByRole("button", { + name: `Organization: ${MockOrganization2.display_name}`, + }), + ).toBeVisible(); + }, +}; + // Mutable permissions let play functions change authorization across refetches. // The story-local QueryClient exposes those refetches; the preview client's // instance is inaccessible and uses infinite stale time. diff --git a/site/src/pages/AgentsPage/components/AgentCreateForm.tsx b/site/src/pages/AgentsPage/components/AgentCreateForm.tsx index 643b92a99c1..5c3517838f4 100644 --- a/site/src/pages/AgentsPage/components/AgentCreateForm.tsx +++ b/site/src/pages/AgentsPage/components/AgentCreateForm.tsx @@ -48,6 +48,9 @@ import { getModelSelectorHelp } from "./ModelSelectorHelp"; /** @internal Exported for testing. */ export const emptyInputStorageKey = "agents.empty-input"; +/** @internal Exported for testing. */ +export const selectedOrganizationIdStorageKey = + "agents.selected-organization-id"; const selectedWorkspaceIdStorageKey = "agents.selected-workspace-id"; const lastModelConfigIDStorageKey = "agents.last-model-config-id"; @@ -165,8 +168,6 @@ export const AgentCreateForm: FC = ({ const [initialLastModelConfigID] = useState(() => { return localStorage.getItem(lastModelConfigIDStorageKey) ?? ""; }); - const initialOrg = - organizations.find((o) => o.is_default) ?? organizations[0]; // effectiveWorkspaceId nulls a stored selection outside the effective org's // filtered workspace list without deleting it. Preserve the stored value // because the permitted-organizations query may resolve after mount and @@ -175,7 +176,16 @@ export const AgentCreateForm: FC = ({ () => localStorage.getItem(selectedWorkspaceIdStorageKey), ); const [selectedOrg, setSelectedOrg] = useState( - null, + () => { + const storedOrganizationId = localStorage.getItem( + selectedOrganizationIdStorageKey, + ); + return ( + organizations.find( + (organization) => organization.id === storedOrganizationId, + ) ?? null + ); + }, ); const [pendingOrgChange, setPendingOrgChange] = useState(null); @@ -195,14 +205,13 @@ export const AgentCreateForm: FC = ({ // selection, its organization list is authoritative so a removed org cannot // remain selected for submission. const permittedOrgs = showOrganizations - ? (permittedOrgsQuery.data ?? organizations) + ? (permittedOrgsQuery.data ?? []) : organizations; // Treat the dashboard org as provisional until permissions resolve so // sends and persisted attachments cannot use an unpermitted org. const orgSelectionSettled = !showOrganizations || permittedOrgsQuery.data !== undefined; - // Prevent effectiveOrg's dashboard fallback from bypassing an empty - // permitted set. + // Keep an authoritative empty permission set distinct from pending data. const noPermittedOrgs = showOrganizations && permittedOrgsQuery.data?.length === 0; const selectedOrgIsPermitted = @@ -228,7 +237,6 @@ export const AgentCreateForm: FC = ({ ? selectedOrg : (permittedOrgs.find((org) => org.is_default) ?? permittedOrgs[0] ?? - initialOrg ?? null); const organizationId = effectiveOrg?.id ?? ""; const mcpServersQuery = useQuery({ @@ -266,6 +274,16 @@ export const AgentCreateForm: FC = ({ setUserMCPServerIds(null); } } + useEffect(() => { + if (!orgSelectionSettled) { + return; + } + if (selectedOrg) { + localStorage.setItem(selectedOrganizationIdStorageKey, selectedOrg.id); + } else { + localStorage.removeItem(selectedOrganizationIdStorageKey); + } + }, [orgSelectionSettled, selectedOrg]); useEffect(() => { if (selectedWorkspaceId === null) { localStorage.removeItem(selectedWorkspaceIdStorageKey); @@ -453,10 +471,11 @@ export const AgentCreateForm: FC = ({ // guarantees completeness. If workspace counts grow large // enough to warrant pagination, this should switch to a // server-side organization: query filter. - const filteredWorkspaces = - showOrganizations && effectiveOrg + const filteredWorkspaces = showOrganizations + ? effectiveOrg ? workspaceOptions.filter((ws) => ws.organization_id === effectiveOrg.id) - : workspaceOptions; + : [] + : workspaceOptions; const effectiveWorkspaceId = selectedWorkspaceId !== null && @@ -609,8 +628,6 @@ export const AgentCreateForm: FC = ({ )} - {/* The pre-settlement list is the unfiltered dashboard fallback; - selecting from it could destroy existing workspace state. */} {showOrganizations && orgSelectionSettled && permittedOrgs.length > 1 && (