From b5f2e89d41dc0e69f75ccd9bf4161b862a37d619 Mon Sep 17 00:00:00 2001 From: McKayla Washburn Date: Mon, 11 Mar 2024 21:05:20 +0000 Subject: [PATCH 01/11] fix: disable auto-create if external auth requirements aren't met --- .../CreateWorkspacePage.tsx | 24 ++++++++++++++++--- .../CreateWorkspacePageView.tsx | 11 +++++---- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx index c2faa52e4b9e2..f619ba78e0dde 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx @@ -39,11 +39,14 @@ const CreateWorkspacePage: FC = () => { const me = useMe(); const navigate = useNavigate(); const [searchParams, setSearchParams] = useSearchParams(); - const mode = getWorkspaceMode(searchParams); - const customVersionId = searchParams.get("version") ?? undefined; const { experiments } = useDashboard(); + const mode = getWorkspaceMode(searchParams); + const customVersionId = searchParams.get("version") ?? undefined; const defaultName = searchParams.get("name"); + const disabledParams = searchParams.get("disable_params")?.split(","); + // `?mode=auto` is set, but a prerequisite was not met, and so has been disabled. + const [isAutoCreateTainted, setIsAutoCreateTainted] = useState(false); const queryClient = useQueryClient(); const autoCreateWorkspaceMutation = useMutation( @@ -128,8 +131,21 @@ const CreateWorkspacePage: FC = () => { } }); + const hasAllRequiredExternalAuth = Boolean( + !isLoadingExternalAuth && + externalAuth?.every((auth) => auth.optional || auth.authenticated), + ); + + if (!isLoadingExternalAuth && !hasAllRequiredExternalAuth) { + // Prevent suddenly resuming auto-mode if the user connects to all of the required + // external auth providers. + setIsAutoCreateTainted(true); + } const autoStartReady = - mode === "auto" && (!autofillEnabled || userParametersQuery.isSuccess); + mode === "auto" && + (!autofillEnabled || userParametersQuery.isSuccess) && + hasAllRequiredExternalAuth && + !isAutoCreateTainted; useEffect(() => { if (autoStartReady) { void automateWorkspaceCreation(); @@ -150,6 +166,7 @@ const CreateWorkspacePage: FC = () => { { externalAuth={externalAuth ?? []} externalAuthPollingState={externalAuthPollingState} startPollingExternalAuth={startPollingExternalAuth} + hasAllRequiredExternalAuth={hasAllRequiredExternalAuth} permissions={permissionsQuery.data as CreateWSPermissions} parameters={realizedParameters as TemplateVersionParameter[]} creatingWorkspace={createWorkspaceMutation.isLoading} diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx index f886d39e85687..27cc51609571e 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx @@ -51,15 +51,17 @@ export const Language = { export interface CreateWorkspacePageViewProps { mode: CreateWorkspaceMode; + defaultName?: string | null; + disabledParams?: string[]; error: unknown; resetMutation: () => 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 +75,10 @@ export interface CreateWorkspacePageViewProps { export const CreateWorkspacePageView: FC = ({ mode, + defaultName, + disabledParams, error, resetMutation, - defaultName, defaultOwner, template, versionId, @@ -90,8 +93,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 +286,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; From 114ebe9a9ea3196b6a270776b392e39e1b117315 Mon Sep 17 00:00:00 2001 From: McKayla Washburn Date: Tue, 12 Mar 2024 16:07:59 +0000 Subject: [PATCH 02/11] =?UTF-8?q?=F0=9F=A7=BC?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx index 27cc51609571e..810f8ca9ef655 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx @@ -4,7 +4,6 @@ import FormHelperText from "@mui/material/FormHelperText"; import TextField from "@mui/material/TextField"; import { type FormikContextType, useFormik } from "formik"; import { type FC, useEffect, useState, useMemo, useCallback } from "react"; -import { useSearchParams } from "react-router-dom"; import * as Yup from "yup"; import type * as TypesGen from "api/typesGenerated"; import { Alert } from "components/Alert/Alert"; From 917047e6f79d42178e6856ac7d4e533df7b4c608 Mon Sep 17 00:00:00 2001 From: McKayla Washburn Date: Tue, 12 Mar 2024 21:11:11 +0000 Subject: [PATCH 03/11] refactor a bit --- .../CreateWorkspacePage.tsx | 27 ++++++++++++------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx index f619ba78e0dde..bf046707e29a6 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx @@ -41,12 +41,10 @@ const CreateWorkspacePage: FC = () => { const [searchParams, setSearchParams] = useSearchParams(); const { experiments } = useDashboard(); - const mode = getWorkspaceMode(searchParams); const customVersionId = searchParams.get("version") ?? undefined; const defaultName = searchParams.get("name"); const disabledParams = searchParams.get("disable_params")?.split(","); - // `?mode=auto` is set, but a prerequisite was not met, and so has been disabled. - const [isAutoCreateTainted, setIsAutoCreateTainted] = useState(false); + const [mode, setMode] = useState(() => getWorkspaceMode(searchParams)); const queryClient = useQueryClient(); const autoCreateWorkspaceMutation = useMutation( @@ -136,16 +134,25 @@ const CreateWorkspacePage: FC = () => { externalAuth?.every((auth) => auth.optional || auth.authenticated), ); - if (!isLoadingExternalAuth && !hasAllRequiredExternalAuth) { + let autoStartReady = + mode === "auto" && + (!autofillEnabled || userParametersQuery.isSuccess) && + hasAllRequiredExternalAuth; + + // `mode=auto` was set, but a prequisite 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. - setIsAutoCreateTainted(true); + setMode("form"); + // Ensure this is always false, so that we don't ever let `automateWorkspaceCreation` + // fire when we're trying to disable it. + autoStartReady = false; } - const autoStartReady = - mode === "auto" && - (!autofillEnabled || userParametersQuery.isSuccess) && - hasAllRequiredExternalAuth && - !isAutoCreateTainted; + useEffect(() => { if (autoStartReady) { void automateWorkspaceCreation(); From 27281e01b6623af6f8525e3da0243ecb6d1cebd0 Mon Sep 17 00:00:00 2001 From: McKayla Washburn Date: Tue, 12 Mar 2024 21:14:39 +0000 Subject: [PATCH 04/11] re --- site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx index bf046707e29a6..98bc18f068a2a 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx @@ -139,7 +139,7 @@ const CreateWorkspacePage: FC = () => { (!autofillEnabled || userParametersQuery.isSuccess) && hasAllRequiredExternalAuth; - // `mode=auto` was set, but a prequisite has failed, and so auto-mode should be abandoned. + // `mode=auto` was set, but a prerequisite has failed, and so auto-mode should be abandoned. if ( mode === "auto" && !isLoadingExternalAuth && From d7482767e3b108ce8bb7686d7955e3c1b9288412 Mon Sep 17 00:00:00 2001 From: McKayla Washburn Date: Thu, 14 Mar 2024 21:46:24 +0000 Subject: [PATCH 05/11] testing --- site/src/api/errors.ts | 42 +++++++++++++++---- .../CreateWorkspacePage.test.tsx | 32 +++++++++++++- .../CreateWorkspacePage.tsx | 27 ++++++++---- 3 files changed, 84 insertions(+), 17 deletions(-) diff --git a/site/src/api/errors.ts b/site/src/api/errors.ts index 7773a84efe8c7..dadb603361491 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 (isApiError(error)) { + return error.response.data.detail; + } + if (isApiErrorResponse(error)) { + return error.detail; + } + if (error instanceof Error) { + return `Please check the developer console for more details.`; + } + return null; +}; diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx index bc48e8e74d81f..f602e8e8a8783 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx @@ -251,15 +251,43 @@ 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"); + + 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(); + }); + 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 81cb7bd782aee..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, @@ -43,6 +44,8 @@ const CreateWorkspacePage: FC = () => { 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( @@ -132,7 +135,7 @@ const CreateWorkspacePage: FC = () => { externalAuth?.every((auth) => auth.optional || auth.authenticated), ); - let autoStartReady = + let autoCreateReady = mode === "auto" && (!autofillEnabled || userParametersQuery.isSuccess) && hasAllRequiredExternalAuth; @@ -148,14 +151,24 @@ const CreateWorkspacePage: FC = () => { setMode("form"); // Ensure this is always false, so that we don't ever let `automateWorkspaceCreation` // fire when we're trying to disable it. - autoStartReady = false; + 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 ( <> @@ -163,9 +176,7 @@ const CreateWorkspacePage: FC = () => { {pageTitle(title)} {loadFormDataError && } - {isLoadingFormData || - isLoadingExternalAuth || - autoCreateWorkspaceMutation.isLoading ? ( + {isLoadingFormData || isLoadingExternalAuth || autoCreateReady ? ( ) : ( { disabledParams={disabledParams} defaultOwner={me} autofillParameters={autofillParameters} - error={createWorkspaceMutation.error} + error={createWorkspaceMutation.error || autoCreateError} resetMutation={createWorkspaceMutation.reset} template={templateQuery.data!} versionId={realizedVersionId} From 2992555a0914b2fe99ade4f73f67a50269185dff Mon Sep 17 00:00:00 2001 From: McKayla Washburn Date: Thu, 14 Mar 2024 21:55:16 +0000 Subject: [PATCH 06/11] =?UTF-8?q?=F0=9F=A4=A1?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- site/src/api/errors.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/api/errors.ts b/site/src/api/errors.ts index dadb603361491..ac2da627d3e6f 100644 --- a/site/src/api/errors.ts +++ b/site/src/api/errors.ts @@ -34,7 +34,7 @@ export const isApiError = (err: unknown): err is ApiError => { export const isApiErrorResponse = (err: unknown): err is ApiErrorResponse => { return ( typeof err === "object" && - err != null && + err !== null && "message" in err && typeof err.message === "string" && (!("detail" in err) || From ef312b2ef70dcc053201d16be5b55a25839331ae Mon Sep 17 00:00:00 2001 From: McKayla Washburn Date: Tue, 19 Mar 2024 20:14:22 +0000 Subject: [PATCH 07/11] :^) --- site/jest.setup.ts | 1 + site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/site/jest.setup.ts b/site/jest.setup.ts index b584fb38414cd..88b5557109f34 100644 --- a/site/jest.setup.ts +++ b/site/jest.setup.ts @@ -50,6 +50,7 @@ global.Blob = Blob as any; 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/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx index f602e8e8a8783..0d48d1e136d42 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx @@ -280,8 +280,8 @@ describe("CreateWorkspacePage", () => { `/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(); From b601ef780454f86c91a137a3cb8eddfe1dfe140f Mon Sep 17 00:00:00 2001 From: McKayla Washburn Date: Tue, 19 Mar 2024 22:11:33 +0000 Subject: [PATCH 08/11] my therapist will definitely be hearing about this commit --- .../pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx index 0d48d1e136d42..5749fb3b7404d 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx @@ -269,7 +269,7 @@ describe("CreateWorkspacePage", () => { const paramValue = "It works!"; const createWorkspaceSpy = jest.spyOn(API, "createWorkspace"); - jest + const externalAuthSpy = jest .spyOn(API, "getTemplateVersionExternalAuth") .mockResolvedValue([MockTemplateVersionExternalAuthGithub]); @@ -286,6 +286,11 @@ describe("CreateWorkspacePage", () => { "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 () => { From dc7c72207e02c3ee1ec88feb4c378dff60126880 Mon Sep 17 00:00:00 2001 From: McKayla Washburn Date: Tue, 19 Mar 2024 22:27:32 +0000 Subject: [PATCH 09/11] true tho --- site/src/components/Alert/ErrorAlert.stories.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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."), }, }; From 92a9962a65bc736ff7aa42b9c33b16624d01789b Mon Sep 17 00:00:00 2001 From: McKayla Washburn Date: Tue, 19 Mar 2024 22:35:31 +0000 Subject: [PATCH 10/11] shame --- site/src/api/errors.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/site/src/api/errors.ts b/site/src/api/errors.ts index ac2da627d3e6f..011cc0da827b3 100644 --- a/site/src/api/errors.ts +++ b/site/src/api/errors.ts @@ -111,14 +111,14 @@ export const getValidationErrorMessage = (error: unknown): string => { }; 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; } - if (error instanceof Error) { - return `Please check the developer console for more details.`; - } return null; }; From c30988cdaaea98bccd0738bee7f91ea1c0323bf7 Mon Sep 17 00:00:00 2001 From: McKayla Washburn Date: Tue, 19 Mar 2024 22:35:53 +0000 Subject: [PATCH 11/11] oejfa;oihegfiou'awrgiuhwaeoug'awikuehjfo;ia --- site/src/api/errors.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/api/errors.ts b/site/src/api/errors.ts index 011cc0da827b3..0d1a3dc203e5b 100644 --- a/site/src/api/errors.ts +++ b/site/src/api/errors.ts @@ -112,7 +112,7 @@ export const getValidationErrorMessage = (error: unknown): string => { export const getErrorDetail = (error: unknown): string | undefined | null => { if (error instanceof Error) { - return `Please check the developer console for more details.`; + return "Please check the developer console for more details."; } if (isApiError(error)) { return error.response.data.detail;