mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(site): set external auth provider polling status individually (#26313)
fixes #22420 ref DEVEX-369 ref DEVEX-269 The bug on `CreateWorkspacePage`, where clicking one external auth provider login button disabled all providers' login buttons, was caused by providers all sharing a single polling status (`"idle" | "polling" | "abandoned"`) in the `useExternalAuth` hook. ## changes - Instead of setting one status across all providers, the polling status in `useExternalAuth` is now tracked for each provider in a record whose keys are the providers' IDs. - The biggest diff is a new Storybook file CreateWorkspacePage.stories.tsx which reproduces the bug behavior from the issue. - Until now we've only had CreateWorkspacePageView.stories.tsx, which isn't able to model the user interactions / API responses needed to verify the bugfix. This file is unchanged. - Also deletes `CreateWorkspacePage`'s `useExternalAuth` hook in favor of the global `useExternalAuth` hook (see #26310) (co-written with Coder Agents)
This commit is contained in:
@@ -5,13 +5,16 @@ import { templateVersionExternalAuth } from "#/api/queries/templates";
|
||||
export type ExternalAuthPollingState = "idle" | "polling" | "abandoned";
|
||||
|
||||
export const useExternalAuth = (versionId: string | undefined) => {
|
||||
const [externalAuthPollingState, setExternalAuthPollingState] =
|
||||
useState<ExternalAuthPollingState>("idle");
|
||||
const [pollingState, setPollingState] = useState<
|
||||
Record<string, ExternalAuthPollingState>
|
||||
>({});
|
||||
|
||||
const startPollingExternalAuth = useCallback(() => {
|
||||
setExternalAuthPollingState("polling");
|
||||
const startPollingExternalAuth = useCallback((providerId: string) => {
|
||||
setPollingState((prev) => ({ ...prev, [providerId]: "polling" }));
|
||||
}, []);
|
||||
|
||||
const isAnyPolling = Object.values(pollingState).some((s) => s === "polling");
|
||||
|
||||
const {
|
||||
data: externalAuth,
|
||||
isPending: isLoadingExternalAuth,
|
||||
@@ -19,37 +22,58 @@ export const useExternalAuth = (versionId: string | undefined) => {
|
||||
} = useQuery({
|
||||
...templateVersionExternalAuth(versionId ?? ""),
|
||||
enabled: Boolean(versionId),
|
||||
refetchInterval: externalAuthPollingState === "polling" ? 1000 : false,
|
||||
refetchInterval: isAnyPolling ? 1000 : false,
|
||||
});
|
||||
|
||||
const allSignedIn = externalAuth?.every((it) => it.authenticated);
|
||||
|
||||
// Stop polling individual providers once they authenticate.
|
||||
useEffect(() => {
|
||||
if (allSignedIn) {
|
||||
setExternalAuthPollingState("idle");
|
||||
if (!externalAuth) {
|
||||
return;
|
||||
}
|
||||
setPollingState((prev) => {
|
||||
let changed = false;
|
||||
const next = { ...prev };
|
||||
for (const auth of externalAuth) {
|
||||
if (auth.authenticated && next[auth.id] === "polling") {
|
||||
next[auth.id] = "idle";
|
||||
changed = true;
|
||||
}
|
||||
}
|
||||
return changed ? next : prev;
|
||||
});
|
||||
}, [externalAuth]);
|
||||
|
||||
// Per-provider 60-second timeout.
|
||||
useEffect(() => {
|
||||
const pollingIds = Object.entries(pollingState)
|
||||
.filter(([, authPollingState]) => authPollingState === "polling")
|
||||
.map(([id]) => id);
|
||||
|
||||
if (pollingIds.length === 0) {
|
||||
return;
|
||||
}
|
||||
|
||||
if (externalAuthPollingState !== "polling") {
|
||||
return;
|
||||
}
|
||||
|
||||
// Poll for a maximum of one minute
|
||||
const quitPolling = setTimeout(
|
||||
() => setExternalAuthPollingState("abandoned"),
|
||||
60_000,
|
||||
const timers = pollingIds.map((id) =>
|
||||
setTimeout(() => {
|
||||
setPollingState((prev) =>
|
||||
prev[id] === "polling" ? { ...prev, [id]: "abandoned" } : prev,
|
||||
);
|
||||
}, 60_000),
|
||||
);
|
||||
|
||||
return () => {
|
||||
clearTimeout(quitPolling);
|
||||
for (const t of timers) {
|
||||
clearTimeout(t);
|
||||
}
|
||||
};
|
||||
}, [externalAuthPollingState, allSignedIn]);
|
||||
}, [pollingState]);
|
||||
|
||||
return {
|
||||
startPollingExternalAuth,
|
||||
externalAuth,
|
||||
externalAuthPollingState,
|
||||
externalAuthPollingState: pollingState,
|
||||
isLoadingExternalAuth,
|
||||
externalAuthError: error,
|
||||
isPollingExternalAuth: externalAuthPollingState === "polling",
|
||||
isPollingExternalAuth: isAnyPolling,
|
||||
};
|
||||
};
|
||||
|
||||
@@ -10,6 +10,7 @@ import {
|
||||
MockTasks,
|
||||
MockTemplate,
|
||||
MockTemplateVersion,
|
||||
MockTemplateVersionExternalAuthAzure,
|
||||
MockTemplateVersionExternalAuthGithub,
|
||||
MockTemplateVersionExternalAuthGithubAuthenticated,
|
||||
MockUserOwner,
|
||||
@@ -387,6 +388,39 @@ export const MissingExternalAuth: Story = {
|
||||
},
|
||||
};
|
||||
|
||||
export const MissingExternalAuthMultipleProviders: Story = {
|
||||
beforeEach: () => {
|
||||
spyOn(API, "getTasks")
|
||||
.mockResolvedValueOnce(MockTasks)
|
||||
.mockResolvedValue([MockNewTaskData, ...MockTasks]);
|
||||
spyOn(API, "createTask").mockResolvedValue(MockTask);
|
||||
spyOn(API, "getTemplateVersionExternalAuth").mockResolvedValue([
|
||||
MockTemplateVersionExternalAuthGithub,
|
||||
MockTemplateVersionExternalAuthAzure,
|
||||
]);
|
||||
// Prevent the auth button from actually opening a popup.
|
||||
spyOn(window, "open").mockReturnValue(null);
|
||||
},
|
||||
play: async ({ canvasElement, step }) => {
|
||||
const canvas = within(canvasElement);
|
||||
|
||||
const githubButton = await canvas.findByRole("button", {
|
||||
name: /connect to github/i,
|
||||
});
|
||||
const azureButton = await canvas.findByRole("button", {
|
||||
name: /connect to azure/i,
|
||||
});
|
||||
|
||||
await step("Click GitHub auth button", async () => {
|
||||
await userEvent.click(githubButton);
|
||||
});
|
||||
|
||||
await step("Azure button remains enabled", () => {
|
||||
expect(azureButton).toBeEnabled();
|
||||
});
|
||||
},
|
||||
};
|
||||
|
||||
export const ExternalAuthError: Story = {
|
||||
beforeEach: () => {
|
||||
spyOn(API, "getTasks")
|
||||
|
||||
@@ -436,14 +436,14 @@ const ExternalAuthButtons: FC<ExternalAuthButtonProps> = ({
|
||||
versionId,
|
||||
missedExternalAuth,
|
||||
}) => {
|
||||
const {
|
||||
startPollingExternalAuth,
|
||||
isPollingExternalAuth,
|
||||
externalAuthPollingState,
|
||||
} = useExternalAuth(versionId);
|
||||
const shouldRetry = externalAuthPollingState === "abandoned";
|
||||
const { startPollingExternalAuth, externalAuthPollingState } =
|
||||
useExternalAuth(versionId);
|
||||
|
||||
return missedExternalAuth.map((auth) => {
|
||||
const isPollingExternalAuth =
|
||||
externalAuthPollingState[auth.id] === "polling";
|
||||
const shouldRetry = externalAuthPollingState[auth.id] === "abandoned";
|
||||
|
||||
return (
|
||||
<div className="flex items-center gap-2" key={auth.id}>
|
||||
<Button
|
||||
@@ -456,7 +456,7 @@ const ExternalAuthButtons: FC<ExternalAuthButtonProps> = ({
|
||||
"_blank",
|
||||
"width=900,height=600",
|
||||
);
|
||||
startPollingExternalAuth();
|
||||
startPollingExternalAuth(auth.id);
|
||||
}}
|
||||
>
|
||||
<Spinner loading={isPollingExternalAuth}>
|
||||
@@ -471,7 +471,7 @@ const ExternalAuthButtons: FC<ExternalAuthButtonProps> = ({
|
||||
<Button
|
||||
variant="outline"
|
||||
size="icon"
|
||||
onClick={startPollingExternalAuth}
|
||||
onClick={() => startPollingExternalAuth(auth.id)}
|
||||
>
|
||||
<RedoIcon />
|
||||
<span className="sr-only">Refresh external auth</span>
|
||||
|
||||
@@ -0,0 +1,220 @@
|
||||
import type { Meta, StoryObj } from "@storybook/react-vite";
|
||||
import { expect, spyOn, userEvent, waitFor, within } from "storybook/test";
|
||||
import { reactRouterParameters } from "storybook-addon-remix-react-router";
|
||||
import { API } from "#/api/api";
|
||||
import {
|
||||
MockTemplate,
|
||||
MockTemplateVersion,
|
||||
MockTemplateVersionExternalAuthAzure,
|
||||
MockTemplateVersionExternalAuthGithub,
|
||||
MockTemplateVersionExternalAuthGithubAuthenticated,
|
||||
MockUserOwner,
|
||||
} from "#/testHelpers/entities";
|
||||
import {
|
||||
withAuthProvider,
|
||||
withDashboardProvider,
|
||||
} from "#/testHelpers/storybook";
|
||||
import CreateWorkspacePage from "./CreateWorkspacePage";
|
||||
|
||||
/**
|
||||
* Mocks API.templateVersionDynamicParameters to immediately send an empty
|
||||
* DynamicParametersResponse so the page renders the form instead of the
|
||||
* loader.
|
||||
*/
|
||||
function mockDynamicParameters() {
|
||||
spyOn(API, "templateVersionDynamicParameters").mockImplementation(
|
||||
(_versionId, _ownerId, callbacks) => {
|
||||
// Fire asynchronously so the component mounts before the message
|
||||
// arrives, matching real WebSocket behavior.
|
||||
setTimeout(() => {
|
||||
callbacks.onMessage({ id: 0, parameters: [], diagnostics: [] });
|
||||
}, 0);
|
||||
|
||||
return { close: () => {} } as unknown as WebSocket;
|
||||
},
|
||||
);
|
||||
}
|
||||
|
||||
const meta: Meta<typeof CreateWorkspacePage> = {
|
||||
title: "pages/CreateWorkspacePage",
|
||||
component: CreateWorkspacePage,
|
||||
decorators: [withAuthProvider, withDashboardProvider],
|
||||
parameters: {
|
||||
layout: "fullscreen",
|
||||
user: MockUserOwner,
|
||||
reactRouter: reactRouterParameters({
|
||||
location: {
|
||||
pathParams: {
|
||||
organization: MockTemplate.organization_name,
|
||||
template: MockTemplate.name,
|
||||
},
|
||||
},
|
||||
routing: {
|
||||
path: "/templates/:organization/:template/workspace",
|
||||
},
|
||||
}),
|
||||
},
|
||||
beforeEach: () => {
|
||||
// Prevent the auth button from actually opening a popup.
|
||||
spyOn(window, "open").mockReturnValue(null);
|
||||
|
||||
// Template, version, and preset queries.
|
||||
spyOn(API, "getTemplateByName").mockResolvedValue(MockTemplate);
|
||||
spyOn(API, "getTemplateVersion").mockResolvedValue(MockTemplateVersion);
|
||||
spyOn(API, "getTemplateVersionPresets").mockResolvedValue(null);
|
||||
spyOn(API, "checkAuthorization").mockResolvedValue({
|
||||
createWorkspaceForAny: true,
|
||||
canUpdateTemplate: false,
|
||||
});
|
||||
|
||||
// Dynamic parameters over WebSocket.
|
||||
mockDynamicParameters();
|
||||
|
||||
// Default: no external auth required.
|
||||
spyOn(API, "getTemplateVersionExternalAuth").mockResolvedValue([]);
|
||||
},
|
||||
};
|
||||
|
||||
export default meta;
|
||||
type Story = StoryObj<typeof CreateWorkspacePage>;
|
||||
|
||||
/**
|
||||
* Renders two unauthenticated external auth providers. Both "Login with"
|
||||
* buttons should be visible and enabled.
|
||||
*/
|
||||
export const MultipleExternalAuth: Story = {
|
||||
beforeEach: () => {
|
||||
spyOn(API, "getTemplateVersionExternalAuth").mockResolvedValue([
|
||||
MockTemplateVersionExternalAuthGithub,
|
||||
MockTemplateVersionExternalAuthAzure,
|
||||
]);
|
||||
},
|
||||
play: async ({ canvasElement }) => {
|
||||
const canvas = within(canvasElement);
|
||||
|
||||
const githubButton = await canvas.findByRole("button", {
|
||||
name: /login with github/i,
|
||||
});
|
||||
const azureButton = await canvas.findByRole("button", {
|
||||
name: /login with azure/i,
|
||||
});
|
||||
|
||||
expect(githubButton).toBeEnabled();
|
||||
expect(azureButton).toBeEnabled();
|
||||
},
|
||||
};
|
||||
|
||||
/**
|
||||
* Clicking one external auth button should only show a loading spinner on
|
||||
* that button. The other provider's button must remain enabled so the user
|
||||
* can authenticate with both without a page refresh.
|
||||
*
|
||||
* This is the regression test for coder/coder#22420.
|
||||
*/
|
||||
export const ClickingOneAuthDoesNotDisableOthers: Story = {
|
||||
beforeEach: () => {
|
||||
spyOn(API, "getTemplateVersionExternalAuth").mockResolvedValue([
|
||||
MockTemplateVersionExternalAuthGithub,
|
||||
MockTemplateVersionExternalAuthAzure,
|
||||
]);
|
||||
},
|
||||
play: async ({ canvasElement, step }) => {
|
||||
const canvas = within(canvasElement);
|
||||
|
||||
const githubButton = await canvas.findByRole("button", {
|
||||
name: /login with github/i,
|
||||
});
|
||||
const azureButton = await canvas.findByRole("button", {
|
||||
name: /login with azure/i,
|
||||
});
|
||||
|
||||
await step("Click GitHub auth button", async () => {
|
||||
await userEvent.click(githubButton);
|
||||
});
|
||||
|
||||
await step("Azure button remains enabled", () => {
|
||||
expect(azureButton).toBeEnabled();
|
||||
});
|
||||
},
|
||||
};
|
||||
|
||||
/**
|
||||
* After the first provider completes authentication and the API starts
|
||||
* returning it as authenticated, its button should be replaced with the
|
||||
* "Authenticated" badge. The second provider's button should still be
|
||||
* clickable.
|
||||
*/
|
||||
export const OneProviderAuthenticated: Story = {
|
||||
beforeEach: () => {
|
||||
spyOn(API, "getTemplateVersionExternalAuth").mockResolvedValue([
|
||||
MockTemplateVersionExternalAuthGithubAuthenticated,
|
||||
MockTemplateVersionExternalAuthAzure,
|
||||
]);
|
||||
},
|
||||
play: async ({ canvasElement, step }) => {
|
||||
const canvas = within(canvasElement);
|
||||
|
||||
await step("GitHub shows authenticated", async () => {
|
||||
await canvas.findByText("Authenticated");
|
||||
});
|
||||
|
||||
await step("Azure login button is still enabled", async () => {
|
||||
const azureButton = await canvas.findByRole("button", {
|
||||
name: /login with azure/i,
|
||||
});
|
||||
expect(azureButton).toBeEnabled();
|
||||
});
|
||||
},
|
||||
};
|
||||
|
||||
/**
|
||||
* Simulates the full two-provider authentication flow: click the first
|
||||
* provider, have polling return it as authenticated, then click the second
|
||||
* provider.
|
||||
*/
|
||||
export const SequentialAuthFlow: Story = {
|
||||
beforeEach: () => {
|
||||
// First call: both unauthenticated.
|
||||
// Subsequent calls: GitHub authenticated (simulating a successful login
|
||||
// during the polling interval).
|
||||
spyOn(API, "getTemplateVersionExternalAuth")
|
||||
.mockResolvedValueOnce([
|
||||
MockTemplateVersionExternalAuthGithub,
|
||||
MockTemplateVersionExternalAuthAzure,
|
||||
])
|
||||
.mockResolvedValue([
|
||||
MockTemplateVersionExternalAuthGithubAuthenticated,
|
||||
MockTemplateVersionExternalAuthAzure,
|
||||
]);
|
||||
},
|
||||
play: async ({ canvasElement, step }) => {
|
||||
const canvas = within(canvasElement);
|
||||
|
||||
await step("Both buttons render initially", async () => {
|
||||
await canvas.findByRole("button", { name: /login with github/i });
|
||||
await canvas.findByRole("button", { name: /login with azure/i });
|
||||
});
|
||||
|
||||
await step("Click GitHub and wait for it to authenticate", async () => {
|
||||
const githubButton = await canvas.findByRole("button", {
|
||||
name: /login with github/i,
|
||||
});
|
||||
await userEvent.click(githubButton);
|
||||
|
||||
// Polling picks up the updated mock that returns GitHub as
|
||||
// authenticated. The "Authenticated" text replaces the button.
|
||||
await waitFor(() => {
|
||||
expect(
|
||||
canvas.queryByRole("button", { name: /login with github/i }),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
|
||||
await step("Azure button is still clickable", async () => {
|
||||
const azureButton = await canvas.findByRole("button", {
|
||||
name: /login with azure/i,
|
||||
});
|
||||
expect(azureButton).toBeEnabled();
|
||||
});
|
||||
},
|
||||
};
|
||||
@@ -15,7 +15,6 @@ import { checkAuthorization } from "#/api/queries/authCheck";
|
||||
import {
|
||||
templateByName,
|
||||
templateVersion,
|
||||
templateVersionExternalAuth,
|
||||
templateVersionPresets,
|
||||
} from "#/api/queries/templates";
|
||||
import { autoCreateWorkspace, createWorkspace } from "#/api/queries/workspaces";
|
||||
@@ -28,6 +27,7 @@ import type {
|
||||
} from "#/api/typesGenerated";
|
||||
import { Loader } from "#/components/Loader/Loader";
|
||||
import { useAuthenticated } from "#/hooks/useAuthenticated";
|
||||
import { useExternalAuth } from "#/hooks/useExternalAuth";
|
||||
import { getInitialParameterValues } from "#/modules/workspaces/DynamicParameter/DynamicParameter";
|
||||
import { generateWorkspaceName } from "#/modules/workspaces/generateWorkspaceName";
|
||||
import { pageTitle } from "#/utils/page";
|
||||
@@ -41,7 +41,6 @@ import {
|
||||
|
||||
const createWorkspaceModes = ["form", "auto", "duplicate"] as const;
|
||||
export type CreateWorkspaceMode = (typeof createWorkspaceModes)[number];
|
||||
type ExternalAuthPollingState = "idle" | "polling" | "abandoned";
|
||||
|
||||
const CreateWorkspacePage: FC = () => {
|
||||
const { organization: organizationName = "default", template: templateName } =
|
||||
@@ -461,50 +460,6 @@ const CreateWorkspacePage: FC = () => {
|
||||
);
|
||||
};
|
||||
|
||||
const useExternalAuth = (versionId: string | undefined) => {
|
||||
const [externalAuthPollingState, setExternalAuthPollingState] =
|
||||
useState<ExternalAuthPollingState>("idle");
|
||||
|
||||
const startPollingExternalAuth = useCallback(() => {
|
||||
setExternalAuthPollingState("polling");
|
||||
}, []);
|
||||
|
||||
const { data: externalAuth, isLoading: isLoadingExternalAuth } = useQuery({
|
||||
...templateVersionExternalAuth(versionId ?? ""),
|
||||
enabled: Boolean(versionId),
|
||||
refetchInterval: externalAuthPollingState === "polling" ? 1000 : false,
|
||||
});
|
||||
|
||||
const allSignedIn = externalAuth?.every((it) => it.authenticated);
|
||||
|
||||
useEffect(() => {
|
||||
if (allSignedIn) {
|
||||
setExternalAuthPollingState("idle");
|
||||
return;
|
||||
}
|
||||
|
||||
if (externalAuthPollingState !== "polling") {
|
||||
return;
|
||||
}
|
||||
|
||||
// Poll for a maximum of one minute
|
||||
const quitPolling = setTimeout(
|
||||
() => setExternalAuthPollingState("abandoned"),
|
||||
60_000,
|
||||
);
|
||||
return () => {
|
||||
clearTimeout(quitPolling);
|
||||
};
|
||||
}, [externalAuthPollingState, allSignedIn]);
|
||||
|
||||
return {
|
||||
startPollingExternalAuth,
|
||||
externalAuth,
|
||||
externalAuthPollingState,
|
||||
isLoadingExternalAuth,
|
||||
};
|
||||
};
|
||||
|
||||
const getAutofillParameters = (
|
||||
urlSearchParams: URLSearchParams,
|
||||
): AutofillBuildParameter[] => {
|
||||
|
||||
@@ -16,7 +16,7 @@ const meta: Meta<typeof CreateWorkspacePageView> = {
|
||||
defaultName: "",
|
||||
defaultOwner: MockUserOwner,
|
||||
externalAuth: [],
|
||||
externalAuthPollingState: "idle",
|
||||
externalAuthPollingState: {},
|
||||
hasAllRequiredExternalAuth: true,
|
||||
mode: "form",
|
||||
parameters: [],
|
||||
|
||||
@@ -66,7 +66,7 @@ interface CreateWorkspacePageViewProps {
|
||||
disabledParams?: string[];
|
||||
error: unknown;
|
||||
externalAuth: TypesGen.TemplateVersionExternalAuth[];
|
||||
externalAuthPollingState: ExternalAuthPollingState;
|
||||
externalAuthPollingState: Record<string, ExternalAuthPollingState>;
|
||||
hasAllRequiredExternalAuth: boolean;
|
||||
hasIgnoredUrlParams?: boolean;
|
||||
mode: CreateWorkspaceMode;
|
||||
@@ -85,7 +85,7 @@ interface CreateWorkspacePageViewProps {
|
||||
) => void;
|
||||
resetMutation: () => void;
|
||||
sendMessage: (message: Record<string, string>, ownerId?: string) => void;
|
||||
startPollingExternalAuth: () => void;
|
||||
startPollingExternalAuth: (providerId: string) => void;
|
||||
owner: TypesGen.MinimalUser;
|
||||
setOwner: (user: TypesGen.MinimalUser) => void;
|
||||
}
|
||||
@@ -588,9 +588,11 @@ export const CreateWorkspacePageView: FC<CreateWorkspacePageViewProps> = ({
|
||||
key={auth.id}
|
||||
error={error}
|
||||
auth={auth}
|
||||
isLoading={externalAuthPollingState === "polling"}
|
||||
onStartPolling={startPollingExternalAuth}
|
||||
displayRetry={externalAuthPollingState === "abandoned"}
|
||||
isLoading={externalAuthPollingState[auth.id] === "polling"}
|
||||
onStartPolling={() => startPollingExternalAuth(auth.id)}
|
||||
displayRetry={
|
||||
externalAuthPollingState[auth.id] === "abandoned"
|
||||
}
|
||||
/>
|
||||
))}
|
||||
</div>
|
||||
|
||||
@@ -3721,6 +3721,16 @@ export const MockTemplateVersionExternalAuthGithubAuthenticated: TypesGen.Templa
|
||||
display_name: "GitHub",
|
||||
};
|
||||
|
||||
export const MockTemplateVersionExternalAuthAzure: TypesGen.TemplateVersionExternalAuth =
|
||||
{
|
||||
id: "azure",
|
||||
type: "azure",
|
||||
authenticate_url: "https://example.com/external-auth/azure",
|
||||
authenticated: false,
|
||||
display_icon: "/icon/azure.svg",
|
||||
display_name: "Azure",
|
||||
};
|
||||
|
||||
export const MockDeploymentStats: TypesGen.DeploymentStats = {
|
||||
aggregated_from: "2023-03-06T19:08:55.211625Z",
|
||||
collected_at: "2023-03-06T19:12:55.211625Z",
|
||||
|
||||
Reference in New Issue
Block a user