Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
aslilac merged 15 commits intomainfrompause-auto-mode
Mar 19, 2024
Merged
Show file tree
Hide file tree
Changes fromall commits
Commits
Show all changes
15 commits
Select commitHold shift + click to select a range
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletionssite/jest.setup.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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", {
Expand Down
42 changes: 35 additions & 7 deletionssite/src/api/errors.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 =>
Expand DownExpand Up@@ -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;
Expand All@@ -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;
};
2 changes: 1 addition & 1 deletionsite/src/components/Alert/ErrorAlert.stories.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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."),
},
};
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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!";
Expand Down
56 changes: 46 additions & 10 deletionssite/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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,
Expand DownExpand Up@@ -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(
Expand DownExpand Up@@ -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}
Expand Down
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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";
Expand DownExpand Up@@ -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;
Expand All@@ -73,9 +74,10 @@ export interface CreateWorkspacePageViewProps {

export const CreateWorkspacePageView: FC<CreateWorkspacePageViewProps> = ({
mode,
defaultName,
disabledParams,
error,
resetMutation,
defaultName,
defaultOwner,
template,
versionId,
Expand All@@ -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(),
Expand DownExpand Up@@ -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;

Expand Down
[8]ページ先頭
©2009-2025 Movatter.jp