Skip to content

chore: add e2e test against an external auth provider during workspace creation #12985

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 18, 2024
Merged

chore: add e2e test against an external auth provider during workspace creation #12985

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
638ce38
Set id on external auth button for easier targeting in tests
dannykopping Apr 17, 2024
e150061
Add e2e test for external auth in workspace creation
dannykopping Apr 17, 2024
eb40352
Clear external auth link between tests to force each external auth te…
dannykopping Apr 18, 2024
5a9ddaf
Review feedback
dannykopping Apr 18, 2024
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
52 changes: 52 additions & 0 deletions site/e2e/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import {
type Resource,
Response,
type RichParameter,
type ExternalAuthProviderResource,
} from "./provisionerGenerated";

// requiresEnterpriseLicense will skip the test if we're not running with an enterprise license
Expand All @@ -49,6 +50,7 @@ export const createWorkspace = async (
templateName: string,
richParameters: RichParameter[] = [],
buildParameters: WorkspaceBuildParameter[] = [],
useExternalAuthProvider: string | undefined = undefined,
Comment on lines 50 to +53
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that with the new parameter, the function is getting a little unwieldy, and if you're looking at things from the consumer side, it's hard to tell which of the five arguments correspond to what

Maybe this could be refactored to an object, so that we have sort of have named arguments?

type CreateWorkspaceInputs = Readonly<{
  // Required properties
  page: Page;
  templateName: string;
  
  // Optional properties
  richParameters?: readonly RichParameter[];
  buildParameters?: readonly WorkspaceBuildParameter[];
  externalAuthProvider?: string;
}>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree in principle, but I think 5 params is still manageable and not worth refactoring the 26 usages of this function IMHO. Happy to discuss.

): Promise<string> => {
await page.goto(`/templates/${templateName}/workspace`, {
waitUntil: "domcontentloaded",
Expand All @@ -59,6 +61,25 @@ export const createWorkspace = async (
await page.getByLabel("name").fill(name);

await fillParameters(page, richParameters, buildParameters);

if (useExternalAuthProvider !== undefined) {
// Create a new context for the popup which will be created when clicking the button
const popupPromise = page.waitForEvent("popup");

// Find the "Login with <Provider>" button
const externalAuthLoginButton = page.locator(
"#external-auth-" + useExternalAuthProvider,
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless of the main way we end up selecting the element, I would recommend making sure the selector asserts that the element we're grabbing is actually exposed to the user as a button (ideally through something like page.getByRole)

It's possible to add click behavior to non-button elements, but that can get janky quick. Changing the selector would add an extra security net to ensure that users can use all the behavior that buttons bake in automatically

await expect(externalAuthLoginButton).toBeVisible();

// Click it
await externalAuthLoginButton.click();

// Wait for authentication to occur
const popup = await popupPromise;
await popup.waitForSelector("text=You are now authenticated.");
}

await page.getByTestId("form-submit").click();

await expectUrl(page).toHavePathName("/@admin/" + name);
Expand Down Expand Up @@ -648,6 +669,37 @@ export const echoResponsesWithParameters = (
};
};

export const echoResponsesWithExternalAuth = (
providers: ExternalAuthProviderResource[],
): EchoProvisionerResponses => {
return {
parse: [
{
parse: {},
},
],
plan: [
{
plan: {
externalAuthProviders: providers,
},
},
],
apply: [
{
apply: {
externalAuthProviders: providers,
resources: [
{
name: "example",
},
],
},
},
],
};
};

export const fillParameters = async (
page: Page,
richParameters: RichParameter[] = [],
Expand Down
2 changes: 1 addition & 1 deletion site/e2e/playwright.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ export default defineConfig({
// Tests for Deployment / User Authentication / OIDC
CODER_OIDC_ISSUER_URL: "https://accounts.google.com",
CODER_OIDC_EMAIL_DOMAIN: "coder.com",
CODER_OIDC_CLIENT_ID: "1234567890", // FIXME: https://github.com/coder/coder/issues/12585
CODER_OIDC_CLIENT_ID: "1234567890",
CODER_OIDC_CLIENT_SECRET: "1234567890Secret",
CODER_OIDC_ALLOW_SIGNUPS: "false",
CODER_OIDC_SIGN_IN_TEXT: "Hello",
Expand Down
36 changes: 35 additions & 1 deletion site/e2e/tests/externalAuth.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,13 @@ import type { Endpoints } from "@octokit/types";
import { test } from "@playwright/test";
import type { ExternalAuthDevice } from "api/typesGenerated";
import { gitAuth } from "../constants";
import { Awaiter, createServer } from "../helpers";
import {
Awaiter,
createServer,
createTemplate,
createWorkspace,
echoResponsesWithExternalAuth,
} from "../helpers";
import { beforeCoderTest } from "../hooks";

test.beforeEach(({ page }) => beforeCoderTest(page));
Expand Down Expand Up @@ -81,6 +87,34 @@ test("external auth web", async ({ baseURL, page }) => {
await page.waitForSelector("text=You've authenticated with GitHub!");
});

test("successful external auth from workspace", async ({ baseURL, page }) => {
// Setup the OIDC mock server
const srv = await createServer(gitAuth.webPort);
srv.use(gitAuth.validatePath, (req, res) => {
res.write(JSON.stringify(ghUser));
res.end();
});
srv.use(gitAuth.tokenPath, (req, res) => {
res.write(JSON.stringify({ access_token: "hello-world" }));
res.end();
});
srv.use(gitAuth.authPath, (req, res) => {
res.redirect(
`${baseURL}/external-auth/${gitAuth.webProvider}/callback?code=1234&state=` +
req.query.state,
);
});

const templateName = await createTemplate(
page,
echoResponsesWithExternalAuth([
{ id: gitAuth.webProvider, optional: false },
]),
);

await createWorkspace(page, templateName, [], [], gitAuth.webProvider);
});

const ghUser: Endpoints["GET /user"]["response"]["data"] = {
login: "kylecarbs",
id: 7122116,
Expand Down
1 change: 1 addition & 0 deletions site/src/pages/CreateWorkspacePage/ExternalAuthButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export const ExternalAuthButton: FC<ExternalAuthButtonProps> = ({
<>
<div css={{ display: "flex", alignItems: "center", gap: 8 }}>
<LoadingButton
id={"external-auth-" + auth.id}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is just for testing purposes, then probably data-testid.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, IDs are mainly supposed to be used for associating HTML elements with other elements, or associating elements with browser URL state

I want to say that if you need some kind of "ID-like value"/testing hook for your elements, and the value doesn't have any functionality for the end-user, data attributes (like data-testid) are the way to go

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another concern is that HTML IDs have to be globally unique. If multiple HTML elements have the same ID, that can cause some nasty jank (e.g., clicking a form input, and a different input getting highlighted)

React has a built-in hook for making it easy to have unique IDs, even when you're working with reusable components
https://react.dev/reference/react/useId

fullWidth
loading={isLoading}
variant="contained"
Expand Down
Loading