diff --git a/site/jest.setup.ts b/site/jest.setup.ts index 605b366473870..946cf98890326 100644 --- a/site/jest.setup.ts +++ b/site/jest.setup.ts @@ -40,6 +40,7 @@ jest.mock("contexts/useProxyLatency", () => ({ global.scrollTo = jest.fn(); window.HTMLElement.prototype.scrollIntoView = jest.fn(); +window.open = jest.fn(); // Polyfill the getRandomValues that is used on utils/random.ts Object.defineProperty(global.self, "crypto", { diff --git a/site/src/api/errors.ts b/site/src/api/errors.ts index 7773a84efe8c7..0d1a3dc203e5b 100644 --- a/site/src/api/errors.ts +++ b/site/src/api/errors.ts @@ -24,7 +24,26 @@ export type ApiError = AxiosError & { }; export const isApiError = (err: unknown): err is ApiError => { - return axios.isAxiosError(err) && err.response !== undefined; + return ( + axios.isAxiosError(err) && + err.response !== undefined && + isApiErrorResponse(err.response.data) + ); +}; + +export const isApiErrorResponse = (err: unknown): err is ApiErrorResponse => { + return ( + typeof err === "object" && + err !== null && + "message" in err && + typeof err.message === "string" && + (!("detail" in err) || + err.detail === undefined || + typeof err.detail === "string") && + (!("validations" in err) || + err.validations === undefined || + Array.isArray(err.validations)) + ); }; export const hasApiFieldErrors = (error: ApiError): boolean => @@ -67,6 +86,9 @@ export const getErrorMessage = ( if (isApiError(error) && error.response.data.message) { return error.response.data.message; } + if (isApiErrorResponse(error)) { + return error.message; + } // if error is a non-empty string if (error && typeof error === "string") { return error; @@ -88,9 +110,15 @@ export const getValidationErrorMessage = (error: unknown): string => { return validationErrors.map((error) => error.detail).join("\n"); }; -export const getErrorDetail = (error: unknown): string | undefined | null => - isApiError(error) - ? error.response.data.detail - : error instanceof Error - ? `Please check the developer console for more details.` - : null; +export const getErrorDetail = (error: unknown): string | undefined | null => { + if (error instanceof Error) { + return "Please check the developer console for more details."; + } + if (isApiError(error)) { + return error.response.data.detail; + } + if (isApiErrorResponse(error)) { + return error.detail; + } + return null; +}; diff --git a/site/src/components/Alert/ErrorAlert.stories.tsx b/site/src/components/Alert/ErrorAlert.stories.tsx index 21e63959c6a88..c31c72ed1b660 100644 --- a/site/src/components/Alert/ErrorAlert.stories.tsx +++ b/site/src/components/Alert/ErrorAlert.stories.tsx @@ -55,6 +55,6 @@ export const WithActionAndDismiss: Story = { export const WithNonApiError: Story = { args: { - error: new Error("Non API error here"), + error: new Error("Everything has gone horribly, devastatingly wrong."), }, }; diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx index bc48e8e74d81f..5749fb3b7404d 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx @@ -251,15 +251,48 @@ describe("CreateWorkspacePage", () => { MockOrganization.id, "me", expect.objectContaining({ - template_id: MockTemplate.id, + template_version_id: MockTemplate.active_version_id, rich_parameter_values: [ - expect.objectContaining({ name: param, value: paramValue }), + expect.objectContaining({ + name: param, + source: "url", + value: paramValue, + }), ], }), ); }); }); + it("disables mode=auto if a required external auth provider is not connected", async () => { + const param = "first_parameter"; + const paramValue = "It works!"; + const createWorkspaceSpy = jest.spyOn(API, "createWorkspace"); + + const externalAuthSpy = jest + .spyOn(API, "getTemplateVersionExternalAuth") + .mockResolvedValue([MockTemplateVersionExternalAuthGithub]); + + renderWithAuth(, { + route: + "/templates/" + + MockTemplate.name + + `/workspace?param.${param}=${paramValue}&mode=auto`, + path: "/templates/:template/workspace", + }); + await waitForLoaderToBeRemoved(); + + const warning = + "This template requires an external authentication provider that is not connected."; + expect(await screen.findByText(warning)).toBeInTheDocument(); + expect(createWorkspaceSpy).not.toBeCalled(); + + // We don't need to do this on any other tests out of hundreds of very, very, + // very similar tests, and yet here, I find it to be absolutely necessary for + // some reason that I certainly do not understand. - Kayla + externalAuthSpy.mockReset(); + }); + it("auto create a workspace if uses mode=auto and version=version-id", async () => { const param = "first_parameter"; const paramValue = "It works!"; diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx index 5e0591cfdf92d..b885b11d32f6c 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx @@ -3,6 +3,7 @@ import { Helmet } from "react-helmet-async"; import { useMutation, useQuery, useQueryClient } from "react-query"; import { useNavigate, useParams, useSearchParams } from "react-router-dom"; import { getUserParameters } from "api/api"; +import type { ApiErrorResponse } from "api/errors"; import { checkAuthorization } from "api/queries/authCheck"; import { richParameters, @@ -37,11 +38,14 @@ const CreateWorkspacePage: FC = () => { const { user: me, organizationId } = useAuthenticated(); const navigate = useNavigate(); const [searchParams, setSearchParams] = useSearchParams(); - const mode = getWorkspaceMode(searchParams); - const customVersionId = searchParams.get("version") ?? undefined; const { experiments } = useDashboard(); + const customVersionId = searchParams.get("version") ?? undefined; const defaultName = searchParams.get("name"); + const disabledParams = searchParams.get("disable_params")?.split(","); + const [mode, setMode] = useState(() => getWorkspaceMode(searchParams)); + const [autoCreateError, setAutoCreateError] = + useState(null); const queryClient = useQueryClient(); const autoCreateWorkspaceMutation = useMutation( @@ -126,13 +130,45 @@ const CreateWorkspacePage: FC = () => { } }); - const autoStartReady = - mode === "auto" && (!autofillEnabled || userParametersQuery.isSuccess); + const hasAllRequiredExternalAuth = Boolean( + !isLoadingExternalAuth && + externalAuth?.every((auth) => auth.optional || auth.authenticated), + ); + + let autoCreateReady = + mode === "auto" && + (!autofillEnabled || userParametersQuery.isSuccess) && + hasAllRequiredExternalAuth; + + // `mode=auto` was set, but a prerequisite has failed, and so auto-mode should be abandoned. + if ( + mode === "auto" && + !isLoadingExternalAuth && + !hasAllRequiredExternalAuth + ) { + // Prevent suddenly resuming auto-mode if the user connects to all of the required + // external auth providers. + setMode("form"); + // Ensure this is always false, so that we don't ever let `automateWorkspaceCreation` + // fire when we're trying to disable it. + autoCreateReady = false; + // Show an error message to explain _why_ the workspace was not created automatically. + const subject = + externalAuth?.length === 1 + ? "an external authentication provider that is" + : "external authentication providers that are"; + setAutoCreateError({ + message: `This template requires ${subject} not connected.`, + detail: + "Auto-creation has been disabled. Please connect all required external authentication providers before continuing.", + }); + } + useEffect(() => { - if (autoStartReady) { + if (autoCreateReady) { void automateWorkspaceCreation(); } - }, [automateWorkspaceCreation, autoStartReady]); + }, [automateWorkspaceCreation, autoCreateReady]); return ( <> @@ -140,23 +176,23 @@ const CreateWorkspacePage: FC = () => { {pageTitle(title)} {loadFormDataError && } - {isLoadingFormData || - isLoadingExternalAuth || - autoCreateWorkspaceMutation.isLoading ? ( + {isLoadingFormData || isLoadingExternalAuth || autoCreateReady ? ( ) : ( void; - defaultName?: string | null; defaultOwner: TypesGen.User; template: TypesGen.Template; versionId?: string; externalAuth: TypesGen.TemplateVersionExternalAuth[]; externalAuthPollingState: ExternalAuthPollingState; startPollingExternalAuth: () => void; + hasAllRequiredExternalAuth: boolean; parameters: TypesGen.TemplateVersionParameter[]; autofillParameters: AutofillBuildParameter[]; permissions: CreateWSPermissions; @@ -73,9 +74,10 @@ export interface CreateWorkspacePageViewProps { export const CreateWorkspacePageView: FC = ({ mode, + defaultName, + disabledParams, error, resetMutation, - defaultName, defaultOwner, template, versionId, @@ -90,8 +92,6 @@ export const CreateWorkspacePageView: FC = ({ onCancel, }) => { const [owner, setOwner] = useState(defaultOwner); - const [searchParams] = useSearchParams(); - const disabledParamsList = searchParams?.get("disable_params")?.split(","); const requiresExternalAuth = externalAuth.some((auth) => !auth.authenticated); const [suggestedName, setSuggestedName] = useState(() => generateWorkspaceName(), @@ -285,7 +285,7 @@ export const CreateWorkspacePageView: FC = ({ const parameterField = `rich_parameter_values.${index}`; const parameterInputName = `${parameterField}.value`; const isDisabled = - disabledParamsList?.includes( + disabledParams?.includes( parameter.name.toLowerCase().replace(/ /g, "_"), ) || creatingWorkspace;