Skip to content

Commit c936319

Browse files
aslilacaslilac
committed
Revert "refactor(site): verify external auth before display ws form (#11777)"
This reverts commit 6145da8.
1 parent fbd436c commit c936319

10 files changed

+358
-482
lines changed

‎site/src/pages/CreateWorkspacePage/CreateWorkspacePage.test.tsx

Lines changed: 59 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -6,21 +6,21 @@ import {
66
MockUser,
77
MockWorkspace,
88
MockWorkspaceQuota,
9+
MockWorkspaceRequest,
910
MockWorkspaceRichParametersRequest,
1011
MockTemplateVersionParameter1,
1112
MockTemplateVersionParameter2,
1213
MockTemplateVersionParameter3,
1314
MockTemplateVersionExternalAuthGithub,
1415
MockOrganization,
16+
MockTemplateVersionExternalAuthGithubAuthenticated,
1517
} from "testHelpers/entities";
1618
import {
1719
renderWithAuth,
1820
waitForLoaderToBeRemoved,
1921
} from "testHelpers/renderHelpers";
2022
import CreateWorkspacePage from "./CreateWorkspacePage";
2123
import { Language } from "./CreateWorkspacePageView";
22-
import { server } from "testHelpers/server";
23-
import { rest } from "msw";
2424

2525
const nameLabelText = "Workspace Name";
2626
const createWorkspaceText = "Create Workspace";
@@ -157,6 +157,63 @@ describe("CreateWorkspacePage", () => {
157157
expect(validationError).toBeInTheDocument();
158158
});
159159

160+
it("external auth authenticates and succeeds", async () => {
161+
jest
162+
.spyOn(API, "getWorkspaceQuota")
163+
.mockResolvedValueOnce(MockWorkspaceQuota);
164+
jest
165+
.spyOn(API, "getUsers")
166+
.mockResolvedValueOnce({ users: [MockUser], count: 1 });
167+
jest.spyOn(API, "createWorkspace").mockResolvedValueOnce(MockWorkspace);
168+
jest
169+
.spyOn(API, "getTemplateVersionExternalAuth")
170+
.mockResolvedValue([MockTemplateVersionExternalAuthGithub]);
171+
172+
renderCreateWorkspacePage();
173+
await waitForLoaderToBeRemoved();
174+
175+
const nameField = await screen.findByLabelText(nameLabelText);
176+
// have to use fireEvent b/c userEvent isn't cleaning up properly between tests
177+
fireEvent.change(nameField, {
178+
target: { value: "test" },
179+
});
180+
181+
const githubButton = await screen.findByText("Login with GitHub");
182+
await userEvent.click(githubButton);
183+
184+
jest
185+
.spyOn(API, "getTemplateVersionExternalAuth")
186+
.mockResolvedValue([MockTemplateVersionExternalAuthGithubAuthenticated]);
187+
188+
await screen.findByText("Authenticated with GitHub");
189+
190+
const submitButton = screen.getByText(createWorkspaceText);
191+
await userEvent.click(submitButton);
192+
193+
await waitFor(() =>
194+
expect(API.createWorkspace).toBeCalledWith(
195+
MockUser.organization_ids[0],
196+
MockUser.id,
197+
expect.objectContaining({
198+
...MockWorkspaceRequest,
199+
}),
200+
),
201+
);
202+
});
203+
204+
it("external auth: errors if unauthenticated", async () => {
205+
jest
206+
.spyOn(API, "getTemplateVersionExternalAuth")
207+
.mockResolvedValueOnce([MockTemplateVersionExternalAuthGithub]);
208+
209+
renderCreateWorkspacePage();
210+
await waitForLoaderToBeRemoved();
211+
212+
await screen.findByText(
213+
"To create a workspace using the selected template, please ensure you are authenticated with all the external providers listed below.",
214+
);
215+
});
216+
160217
it("auto create a workspace if uses mode=auto", async () => {
161218
const param = "first_parameter";
162219
const paramValue = "It works!";
@@ -231,46 +288,4 @@ describe("CreateWorkspacePage", () => {
231288
expect(warningMessage).toHaveTextContent(Language.duplicationWarning);
232289
expect(nameInput).toHaveValue(`${MockWorkspace.name}-copy`);
233290
});
234-
235-
it("displays the form after connecting to all the external services", async () => {
236-
jest.spyOn(window, "open").mockImplementation(() => null);
237-
const user = userEvent.setup();
238-
const notAuthenticatedExternalAuth = {
239-
...MockTemplateVersionExternalAuthGithub,
240-
authenticated: false,
241-
};
242-
server.use(
243-
rest.get(
244-
"/api/v2/templateversions/:versionId/external-auth",
245-
(req, res, ctx) => {
246-
return res(ctx.json([notAuthenticatedExternalAuth]));
247-
},
248-
),
249-
);
250-
renderCreateWorkspacePage();
251-
252-
await screen.findByText("External authentication");
253-
expect(screen.queryByRole("form")).not.toBeInTheDocument();
254-
255-
const connectButton = screen.getByRole("button", {
256-
name: /connect/i,
257-
});
258-
server.use(
259-
rest.get(
260-
"/api/v2/templateversions/:versionId/external-auth",
261-
(req, res, ctx) => {
262-
const authenticatedExternalAuth = {
263-
...MockTemplateVersionExternalAuthGithub,
264-
authenticated: true,
265-
};
266-
return res(ctx.json([authenticatedExternalAuth]));
267-
},
268-
),
269-
);
270-
await user.click(connectButton);
271-
// TODO: Consider improving the timeout by simulating react-query polling.
272-
// Current implementation could not achieve this, further research is
273-
// needed.
274-
await screen.findByRole("form", undefined, { timeout: 10_000 });
275-
});
276291
});

‎site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.stories.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ export const Parameters: Story = {
108108
},
109109
};
110110

111-
export const RequiresExternalAuth: Story = {
111+
export const ExternalAuth: Story = {
112112
args: {
113113
externalAuth: [
114114
{

‎site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx

Lines changed: 116 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,11 @@ import {
2525
getInitialRichParameterValues,
2626
useValidationSchemaForRichParameters,
2727
} from "utils/richParameters";
28+
import {
29+
ImmutableTemplateParametersSection,
30+
MutableTemplateParametersSection,
31+
} from "components/TemplateParameters/TemplateParameters";
32+
import { ExternalAuthButton } from "./ExternalAuthButton";
2833
import { ErrorAlert } from "components/Alert/ErrorAlert";
2934
import { Stack } from "components/Stack/Stack";
3035
import { Alert } from "components/Alert/Alert";
@@ -171,48 +176,74 @@ export const CreateWorkspacePageView: FC<CreateWorkspacePageViewProps> = ({
171176
</Stack>
172177
</PageHeader>
173178

174-
{requiresExternalAuth ? (
175-
<ExternalAuthBanner
176-
providers={externalAuth}
177-
pollingState={externalAuthPollingState}
178-
onStartPolling={startPollingExternalAuth}
179-
/>
180-
) : (
181-
<HorizontalForm
182-
name="create-workspace-form"
183-
onSubmit={form.handleSubmit}
184-
css={{ padding: "16px 0" }}
179+
<HorizontalForm onSubmit={form.handleSubmit} css={{ padding: "16px 0" }}>
180+
{Boolean(error) && <ErrorAlert error={error} />}
181+
182+
{mode === "duplicate" && (
183+
<Alert severity="info" dismissible>
184+
{Language.duplicationWarning}
185+
</Alert>
186+
)}
187+
188+
{/* General info */}
189+
<FormSection
190+
title="General"
191+
description={
192+
permissions.createWorkspaceForUser
193+
? "The name of the workspace and its owner. Only admins can create workspace for other users."
194+
: "The name of your new workspace."
195+
}
185196
>
186-
{Boolean(error) && <ErrorAlert error={error} />}
197+
<FormFields>
198+
{versionId && versionId !== template.active_version_id && (
199+
<Stack spacing={1} css={styles.hasDescription}>
200+
<TextField
201+
disabled
202+
fullWidth
203+
value={versionId}
204+
label="Version ID"
205+
/>
206+
<span css={styles.description}>
207+
This parameter has been preset, and cannot be modified.
208+
</span>
209+
</Stack>
210+
)}
187211

188-
{mode === "duplicate" && (
189-
<Alert severity="info" dismissible>
190-
{Language.duplicationWarning}
191-
</Alert>
192-
)}
212+
<TextField
213+
{...getFieldHelpers("name")}
214+
disabled={creatingWorkspace}
215+
// resetMutation facilitates the clearing of validation errors
216+
onChange={onChangeTrimmed(form, resetMutation)}
217+
autoFocus
218+
fullWidth
219+
label="Workspace Name"
220+
/>
193221

194-
{/* General info */}
222+
{permissions.createWorkspaceForUser && (
223+
<UserAutocomplete
224+
value={owner}
225+
onChange={(user) => {
226+
setOwner(user ?? defaultOwner);
227+
}}
228+
label="Owner"
229+
size="medium"
230+
/>
231+
)}
232+
</FormFields>
233+
</FormSection>
234+
235+
{externalAuth && externalAuth.length > 0 && (
195236
<FormSection
196-
title="General"
197-
description={
198-
permissions.createWorkspaceForUser
199-
? "The name of the workspace and its owner. Only admins can create workspace for other users."
200-
: "The name of your new workspace."
201-
}
237+
title="External Authentication"
238+
description="This template requires authentication to external services."
202239
>
203240
<FormFields>
204-
{versionId && versionId !== template.active_version_id && (
205-
<Stack spacing={1} css={styles.hasDescription}>
206-
<TextField
207-
disabled
208-
fullWidth
209-
value={versionId}
210-
label="Version ID"
211-
/>
212-
<span css={styles.description}>
213-
This parameter has been preset, and cannot be modified.
214-
</span>
215-
</Stack>
241+
{requiresExternalAuth && (
242+
<Alert severity="error">
243+
To create a workspace using the selected template, please
244+
ensure you are authenticated with all the external providers
245+
listed below.
246+
</Alert>
216247
)}
217248
<div>
218249
<TextField
@@ -248,54 +279,63 @@ export const CreateWorkspacePageView: FC<CreateWorkspacePageViewProps> = ({
248279
size="medium"
249280
/>
250281
)}
282+
{externalAuth.map((auth) => (
283+
<ExternalAuthButton
284+
key={auth.id}
285+
auth={auth}
286+
isLoading={externalAuthPollingState === "polling"}
287+
onStartPolling={startPollingExternalAuth}
288+
displayRetry={externalAuthPollingState === "abandoned"}
289+
/>
290+
))}
251291
</FormFields>
252292
</FormSection>
293+
)}
253294

254-
{parameters.length > 0 && (
255-
<FormSection
256-
title="Parameters"
257-
description="These are the settings used by your template. Please note that immutable parameters cannot be modified once the workspace is created."
258-
>
259-
{/*
295+
{parameters.length > 0 && (
296+
<FormSection
297+
title="Parameters"
298+
description="These are the settings used by your template. Please note that immutable parameters cannot be modified once the workspace is created."
299+
>
300+
{/*
260301
Opted not to use FormFields in order to increase spacing.
261302
This decision was made because rich parameter inputs are more visually dense than standard text fields.
262303
*/}
263-
<div css={{ display: "flex", flexDirection: "column", gap: 36 }}>
264-
{parameters.map((parameter, index) => {
265-
const parameterField = `rich_parameter_values.${index}`;
266-
const parameterInputName = `${parameterField}.value`;
267-
const isDisabled =
268-
disabledParamsList?.includes(
269-
parameter.name.toLowerCase().replace(/ /g, "_"),
270-
) || creatingWorkspace;
304+
<div css={{ display: "flex", flexDirection: "column", gap: 36 }}>
305+
{parameters.map((parameter, index) => {
306+
const parameterField = `rich_parameter_values.${index}`;
307+
const parameterInputName = `${parameterField}.value`;
308+
const isDisabled =
309+
disabledParamsList?.includes(
310+
parameter.name.toLowerCase().replace(/ /g, "_"),
311+
) || creatingWorkspace;
271312

272-
return (
273-
<RichParameterInput
274-
{...getFieldHelpers(parameterInputName)}
275-
onChange={async (value) => {
276-
await form.setFieldValue(parameterField, {
277-
name: parameter.name,
278-
value,
279-
});
280-
}}
281-
autofillSource={autofillSources[parameter.name]}
282-
key={parameter.name}
283-
parameter={parameter}
284-
disabled={isDisabled}
285-
/>
286-
);
287-
})}
288-
</div>
289-
</FormSection>
290-
)}
313+
return (
314+
<RichParameterInput
315+
{...getFieldHelpers(parameterInputName)}
316+
onChange={async (value) => {
317+
await form.setFieldValue(parameterField, {
318+
name: parameter.name,
319+
value,
320+
});
321+
}}
322+
autofillSource={autofillSources[parameter.name]}
323+
key={parameter.name}
324+
parameter={parameter}
325+
disabled={isDisabled}
326+
/>
327+
);
328+
})}
329+
</div>
330+
</FormSection>
331+
)}
291332

292-
<FormFooter
293-
onCancel={onCancel}
294-
isLoading={creatingWorkspace}
295-
submitLabel="Create Workspace"
296-
/>
297-
</HorizontalForm>
298-
)}
333+
<FormFooter
334+
onCancel={onCancel}
335+
isLoading={creatingWorkspace}
336+
submitLabel="Create Workspace"
337+
/>
338+
</HorizontalForm>
299339
</Margins>
300340
);
301341
};

0 commit comments

Comments
 (0)