fix: disable auto-create if external auth requirements aren't met (coder#12538)

aslilac · web-flow · commit 9028717c9ba0 · 2024-03-19T16:42:40.000-06:00
diff --git 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
@@ -24,7 +24,26 @@ export type ApiError = AxiosError<ApiErrorResponse> & {
 };
 
 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
@@ -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
@@ -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(<CreateWorkspacePage />, {
+      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
@@ -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<ApiErrorResponse | null>(null);
 
   const queryClient = useQueryClient();
   const autoCreateWorkspaceMutation = useMutation(
@@ -126,37 +130,69 @@ 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 (
     <>
       <Helmet>
         <title>{pageTitle(title)}</title>
       </Helmet>
       {loadFormDataError && <ErrorAlert error={loadFormDataError} />}
-      {isLoadingFormData ||
-      isLoadingExternalAuth ||
-      autoCreateWorkspaceMutation.isLoading ? (
+      {isLoadingFormData || isLoadingExternalAuth || autoCreateReady ? (
         <Loader />
       ) : (
         <CreateWorkspacePageView
           mode={mode}
           defaultName={defaultName}
+          disabledParams={disabledParams}
           defaultOwner={me}
           autofillParameters={autofillParameters}
-          error={createWorkspaceMutation.error}
+          error={createWorkspaceMutation.error || autoCreateError}
           resetMutation={createWorkspaceMutation.reset}
           template={templateQuery.data!}
           versionId={realizedVersionId}
           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
@@ -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";
@@ -51,15 +50,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 +74,10 @@ export interface CreateWorkspacePageViewProps {
 
 export const CreateWorkspacePageView: FC<CreateWorkspacePageViewProps> = ({
   mode,
+  defaultName,
+  disabledParams,
   error,
   resetMutation,
-  defaultName,
   defaultOwner,
   template,
   versionId,
@@ -90,8 +92,6 @@ export const CreateWorkspacePageView: FC<CreateWorkspacePageViewProps> = ({
   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<CreateWorkspacePageViewProps> = ({
                 const parameterField = `rich_parameter_values.${index}`;
                 const parameterInputName = `${parameterField}.value`;
                 const isDisabled =
-                  disabledParamsList?.includes(
+                  disabledParams?.includes(
                     parameter.name.toLowerCase().replace(/ /g, "_"),
                   ) || creatingWorkspace;
 
