From 7283da0b6247f9dc3c22dcce4f4edd8f6839ff5e Mon Sep 17 00:00:00 2001 From: Raunak Bhagat Date: Tue, 13 Oct 2026 23:10:37 +0000 Subject: [PATCH] feat(connectors): set up Microsoft accounts in the connector form (#15966) --- .agents/feature-map/components/connectors.md | 2 +- .../server/documents/draft_credentials.py | 4 + .../server/documents/private_key_types.py | 39 +++++- .../documents/test_pkcs12_credential_value.py | 59 +++++++++ .../components/CredentialFieldsRenderer.tsx | 76 ++++++++--- .../components/CredentialFileField.test.tsx | 122 ++++++++++++++++++ web/src/lib/credentials/utils.ts | 15 +++ .../AddConnectorPage/newAccount.test.ts | 46 +++++++ .../connectors/AddConnectorPage/newAccount.ts | 13 +- .../sections/AuthenticationAccountSection.tsx | 2 + .../onedrive_connector_setup.spec.ts | 41 +++--- .../e2e/pages/OneDriveConnectorSetupPage.ts | 121 ++++------------- 12 files changed, 394 insertions(+), 146 deletions(-) create mode 100644 backend/tests/unit/onyx/server/documents/test_pkcs12_credential_value.py create mode 100644 web/src/lib/credentials/components/CredentialFileField.test.tsx diff --git a/.agents/feature-map/components/connectors.md b/.agents/feature-map/components/connectors.md index 3a1246dd4b..03826ad84f 100644 --- a/.agents/feature-map/components/connectors.md +++ b/.agents/feature-map/components/connectors.md @@ -90,7 +90,7 @@ frontend changes a new connector requires. | GET | `/connector/{google-drive,gmail}/authorize/{credential_id}`, `/callback` | Google-specific OAuth flows in `connector.py`. | | GET | `/connector/oauth/authorize/{source}`, `/callback/{source}`, `/details/{source}` | Generic OAuth flow for every `OAuthConnector` implementation (`server/documents/standard_oauth.py`). | | POST/GET | `/admin/credential/{credential_id}/capability-check`, `/capability-report`, `/admin/credential/capability-reports`, `/admin/credential/{credential_id}/binding-check` | Run and read capability checks, and check credential-bound config fields (`server/documents/credential_capabilities.py`). A run applies the credential use rule (§4.9) to the config it uses. A `connector_specific_config` override needs credential visibility; only a run on the stored config of an existing pair is open to a pairing manager who cannot see the credential. | -| POST/GET | `/admin/connector-checks/runs`, `/admin/connector-checks/runs/{run_id}`, `/admin/connector-checks/runs/{run_id}/cancel` | Draft capability-check runs on an unsaved connector form (`server/documents/capability_check_runs.py`), on a saved credential or the user's draft. A new account's `credential_json` is saved as a draft credential (`Credential.is_draft`), which only its owner sees; the snapshot's `credential_id` names it, and later requests send the values again only when they changed. The add-connector form sends a typed account's values only with a check run or Create, and its binding check names the draft by id once a run saved the current values. The `CLEANUP_STALE_DRAFT_CREDENTIALS` beat task deletes drafts unchanged for 7 days. Cancel stops a run its starter owns; the form calls it when its credential changes. A run starts only when the credential use rule (§4.9) allows the form's config. | +| POST/GET | `/admin/connector-checks/runs`, `/admin/connector-checks/runs/{run_id}`, `/admin/connector-checks/runs/{run_id}/cancel` | Draft capability-check runs on an unsaved connector form (`server/documents/capability_check_runs.py`), on a saved credential or the user's draft. A new account's `credential_json` is saved as a draft credential (`Credential.is_draft`), which only its owner sees; the snapshot's `credential_id` names it, and later requests send the values again only when they changed. The add-connector form sends a typed account's values only with a check run or Create, and its binding check names the draft by id once a run saved the current values. A Microsoft `.pfx` certificate travels in `credential_json` as base64 text, the form the private-key upload stores; `resolve_form_credential` checks it as PKCS#12 (`private_key_types.validate_pkcs12_credential_value`). The `CLEANUP_STALE_DRAFT_CREDENTIALS` beat task deletes drafts unchanged for 7 days. Cancel stops a run its starter owns; the form calls it when its credential changes. A run starts only when the credential use rule (§4.9) allows the form's config. | | POST | `/admin/connector-checks/plan` | The checks a draft run would hold for an unsaved form, each in its state before anything runs (pending, waiting, not applicable). Needs no credential and starts no run. | | GET | `/connector`, `/connector/{connector_id}`, `/indexed-sources` | Read paths, including the anonymous-ish `/connector-status` used by chat surfaces. | diff --git a/backend/onyx/server/documents/draft_credentials.py b/backend/onyx/server/documents/draft_credentials.py index b34cf9d564..63e77db87c 100644 --- a/backend/onyx/server/documents/draft_credentials.py +++ b/backend/onyx/server/documents/draft_credentials.py @@ -21,6 +21,7 @@ from onyx.db.models import Credential, User from onyx.error_handling.error_codes import OnyxErrorCode from onyx.error_handling.exceptions import OnyxError from onyx.server.documents.models import NAMES_A_CREDENTIAL +from onyx.server.documents.private_key_types import validate_pkcs12_credential_value from onyx.utils.encryption import reject_masked_credentials @@ -80,6 +81,9 @@ def resolve_form_credential( try: if credential_json is not None: + # A certificate comes as base64 text, not an upload: check it as + # the upload is checked. + validate_pkcs12_credential_value(source, credential_json) if credential is None: # A new draft, or a lost draft made again. return create_draft_credential( diff --git a/backend/onyx/server/documents/private_key_types.py b/backend/onyx/server/documents/private_key_types.py index e14c83da33..803ec52970 100644 --- a/backend/onyx/server/documents/private_key_types.py +++ b/backend/onyx/server/documents/private_key_types.py @@ -1,9 +1,11 @@ import base64 +import binascii from enum import Enum -from typing import Protocol +from typing import Any, Protocol from fastapi import UploadFile +from onyx.configs.constants import DocumentSource from onyx.error_handling.error_codes import OnyxErrorCode from onyx.error_handling.exceptions import OnyxError from onyx.server.documents.document_utils import validate_pkcs12_content @@ -45,3 +47,38 @@ FILE_TYPE_TO_FILE_PROCESSOR: dict[ PrivateKeyFileTypes.SHAREPOINT_PFX_FILE: process_pkcs12_private_key_file, PrivateKeyFileTypes.ONEDRIVE_PFX_FILE: process_pkcs12_private_key_file, } + + +# The credential key that holds each source's .pfx certificate, as base64. +PKCS12_CREDENTIAL_KEYS: dict[DocumentSource, str] = { + DocumentSource.SHAREPOINT: "sp_private_key", + DocumentSource.ONEDRIVE: "onedrive_private_key", + DocumentSource.TEAMS: "teams_private_key", + DocumentSource.OUTLOOK: "outlook_private_key", +} + + +def validate_pkcs12_credential_value( + source: DocumentSource, credential_json: dict[str, Any] +) -> None: + """A new account typed into a connector form carries its .pfx + certificate as base64 text, the form the private-key upload stores it in. + Checks it as that upload checks the file. + + Raises: + ValueError: the value is not base64, or not a PKCS#12 (.pfx) file. + """ + key = PKCS12_CREDENTIAL_KEYS.get(source) + if key is None: + return + value = credential_json.get(key) + if value is None or value == "": + return + if not isinstance(value, str): + raise ValueError(f"{key} must be the .pfx file's base64 content.") + try: + private_key_bytes = base64.b64decode(value, validate=True) + except (binascii.Error, ValueError) as e: + raise ValueError(f"{key} is not valid base64.") from e + if not validate_pkcs12_content(private_key_bytes): + raise ValueError(f"{key} is not a valid PKCS#12 (.pfx) file.") diff --git a/backend/tests/unit/onyx/server/documents/test_pkcs12_credential_value.py b/backend/tests/unit/onyx/server/documents/test_pkcs12_credential_value.py new file mode 100644 index 0000000000..80b6a61262 --- /dev/null +++ b/backend/tests/unit/onyx/server/documents/test_pkcs12_credential_value.py @@ -0,0 +1,59 @@ +"""A typed account's .pfx certificate, sent as base64 in credential_json, is +checked as the private-key upload checks the file.""" + +import base64 + +import pytest +from cryptography.hazmat.primitives.asymmetric import ec +from cryptography.hazmat.primitives.serialization import BestAvailableEncryption, pkcs12 + +from onyx.configs.constants import DocumentSource +from onyx.server.documents.private_key_types import validate_pkcs12_credential_value + + +def _pfx_base64() -> str: + key = ec.generate_private_key(ec.SECP256R1()) + pfx = pkcs12.serialize_key_and_certificates( + b"onyx", key, None, None, BestAvailableEncryption(b"secret") + ) + return base64.b64encode(pfx).decode("ascii") + + +@pytest.mark.parametrize( + ("source", "key"), + [ + (DocumentSource.SHAREPOINT, "sp_private_key"), + (DocumentSource.ONEDRIVE, "onedrive_private_key"), + (DocumentSource.TEAMS, "teams_private_key"), + (DocumentSource.OUTLOOK, "outlook_private_key"), + ], +) +def test_accepts_a_pfx_certificate(source: DocumentSource, key: str) -> None: + validate_pkcs12_credential_value(source, {key: _pfx_base64()}) + + +def test_accepts_an_account_without_a_certificate() -> None: + validate_pkcs12_credential_value( + DocumentSource.SHAREPOINT, {"sp_client_secret": "secret"} + ) + + +def test_rejects_text_that_is_not_base64() -> None: + with pytest.raises(ValueError, match="not valid base64"): + validate_pkcs12_credential_value( + DocumentSource.SHAREPOINT, {"sp_private_key": "not base64!"} + ) + + +def test_rejects_a_file_that_is_not_pkcs12() -> None: + not_pfx = base64.b64encode(b"just some text, not a certificate").decode() + with pytest.raises(ValueError, match="not a valid PKCS#12"): + validate_pkcs12_credential_value( + DocumentSource.SHAREPOINT, {"sp_private_key": not_pfx} + ) + + +def test_ignores_sources_without_a_certificate() -> None: + validate_pkcs12_credential_value( + DocumentSource.CONFLUENCE, {"sp_private_key": "not base64!"} + ) diff --git a/web/src/lib/credentials/components/CredentialFieldsRenderer.tsx b/web/src/lib/credentials/components/CredentialFieldsRenderer.tsx index 2b5b54dc21..a765a2ce22 100644 --- a/web/src/lib/credentials/components/CredentialFieldsRenderer.tsx +++ b/web/src/lib/credentials/components/CredentialFieldsRenderer.tsx @@ -1,6 +1,6 @@ "use client"; -import { useMemo, useState } from "react"; +import { useEffect, useMemo, useRef, useState } from "react"; import { InputSingleUpload, InputPasswordTypeIn, @@ -22,14 +22,12 @@ import type { IconFunctionComponent, RichStr } from "@opal/types"; import { markdown } from "@opal/utils"; import { getIn, useField, useFormikContext } from "formik"; import { useTranslations } from "next-intl"; -import { - FILE_TYPE_DEFINITIONS, - type TypedFile, -} from "@/lib/connectors/fileTypes"; +import { FILE_TYPE_DEFINITIONS, TypedFile } from "@/lib/connectors/fileTypes"; import { createTypedFile } from "@/lib/connectors/utils"; import { FormikField } from "@/refresh-components/form/FormikField"; import { useCredentialFieldCopy } from "@/lib/credentials/hooks"; import { + fileToBase64, getCredentialFileType, getCredentialSpec, methodFields, @@ -41,12 +39,20 @@ import type { } from "@/lib/credentials/types"; import type { ValidSources } from "@/lib/connectors/types/source"; +/** + * How a file field holds its file: a `TypedFile`, uploaded on its own when + * the account is saved, or the file's base64 text, sent with the account's + * other values (a new account typed into a connector form). + */ +export type CredentialFileValues = "typedFile" | "base64"; + interface CredentialFieldProps { source: ValidSources; fieldKey: string; field: CredentialSpecField; /** The Formik path the field lives under, e.g. `new_account`. */ namePrefix?: string; + fileValues?: CredentialFileValues; } /** @@ -73,6 +79,7 @@ function CredentialField({ fieldKey, field, namePrefix, + fileValues = "typedFile", }: CredentialFieldProps) { const t = useTranslations("admin"); const copy = useCredentialFieldCopy(source)(fieldKey); @@ -88,6 +95,7 @@ function CredentialField({ fieldKey={fieldKey} label={label} description={copy.description} + values={fileValues} /> ); } @@ -186,42 +194,75 @@ interface CredentialFileFieldProps { * backticks shows as code. */ description?: string | RichStr; + /** How the field holds the file. @default "typedFile" */ + values?: CredentialFileValues; } /** - * A key file, such as a .pfx certificate. It stays binary: the field holds a - * `TypedFile`, checked against its type's size and extensions when picked. + * A key file, such as a .pfx certificate, checked against its type's size + * and extensions when picked. The field holds it as a `TypedFile`, or as its + * base64 text with `values="base64"`; either way the bytes stay intact. */ export function CredentialFileField({ name, fieldKey, label, description, + values = "typedFile", }: CredentialFileFieldProps) { const t = useTranslations("common.field"); - const [field, , helpers] = useField(name); + const [field, , helpers] = useField(name); const [fileError, setFileError] = useState(null); + // As base64 text the field cannot name its file, so the picked file is + // kept to show; a cleared or reset field drops it. + const [pickedFile, setPickedFile] = useState(null); + const hasValue: boolean = field.value !== null && field.value !== ""; + useEffect(() => { + if (!hasValue) setPickedFile(null); + }, [hasValue]); + const shownFile: File | null = + field.value instanceof TypedFile + ? field.value.file + : hasValue + ? pickedFile + : null; const fileType = getCredentialFileType(fieldKey); const definition = fileType ? FILE_TYPE_DEFINITIONS[fileType] : null; + // Each pick is numbered; a read that a later pick or a clear overtook is + // dropped, so it cannot bring back an older file. + const latestPickRef = useRef(0); + async function takeFile(file: File | null) { + const pick = ++latestPickRef.current; + const isLatest = () => pick === latestPickRef.current; setFileError(null); - if (!file) { - void helpers.setValue(null); - return; - } + // Until the new file is checked and read, the field holds no file, so + // nothing can send the previous one under the new name. + setPickedFile(null); + void helpers.setValue(null); + if (!file) return; if (!fileType) { setFileError(t("fileValidation.noTypeDefinition", { name: fieldKey })); return; } const typedFile = createTypedFile(file, fieldKey, fileType); const { isValid, errors } = await typedFile.validate(); + if (!isLatest()) return; if (!isValid) { setFileError(errors.join(", ") || t("fileValidation.unknownError")); - void helpers.setValue(null); return; } - void helpers.setValue(typedFile); + let value: TypedFile | string; + try { + value = values === "base64" ? await fileToBase64(file) : typedFile; + } catch { + if (isLatest()) setFileError(t("fileValidation.selectionError")); + return; + } + if (!isLatest()) return; + setPickedFile(file); + void helpers.setValue(value); void helpers.setTouched(true, false); } @@ -239,7 +280,7 @@ export function CredentialFileField({ name={name} data-testid={fieldKey} accept={definition?.validation?.allowedExtensions?.join(",")} - value={field.value?.file ?? null} + value={shownFile} onFileChange={(file) => void takeFile(file)} variant={fileError ? "error" : "primary"} /> @@ -260,6 +301,8 @@ interface CredentialFieldsRendererProps { namePrefix?: string; /** Field keys this renderer leaves out, e.g. ones drawn elsewhere. */ exclude?: ReadonlySet; + /** How file fields hold their file. @default "typedFile" */ + fileValues?: CredentialFileValues; } export function CredentialFieldsRenderer({ @@ -269,6 +312,7 @@ export function CredentialFieldsRenderer({ setAuthMethod, namePrefix, exclude, + fileValues, }: CredentialFieldsRendererProps) { const t = useTranslations("admin"); const { values, setValues, setFieldValue } = @@ -334,6 +378,7 @@ export function CredentialFieldsRenderer({ fieldKey={key} field={field} namePrefix={namePrefix} + fileValues={fileValues} /> ))} @@ -354,6 +399,7 @@ export function CredentialFieldsRenderer({ fieldKey={key} field={field} namePrefix={namePrefix} + fileValues={fileValues} /> ))} diff --git a/web/src/lib/credentials/components/CredentialFileField.test.tsx b/web/src/lib/credentials/components/CredentialFileField.test.tsx new file mode 100644 index 0000000000..4a4cf7df6d --- /dev/null +++ b/web/src/lib/credentials/components/CredentialFileField.test.tsx @@ -0,0 +1,122 @@ +import { render, screen, setupUser, waitFor } from "@tests/setup/test-utils"; +import { Formik, type FormikProps } from "formik"; +import { CredentialFileField } from "@/lib/credentials/components/CredentialFieldsRenderer"; + +jest.mock("@/lib/credentials/utils", () => { + const actual = jest.requireActual("@/lib/credentials/utils"); + return { ...actual, fileToBase64: jest.fn(actual.fileToBase64) }; +}); + +const utils = jest.requireMock("@/lib/credentials/utils"); + +interface CertificateForm { + sp_private_key: string | null; +} + +function renderField(onForm: (form: FormikProps) => void) { + return render( + + initialValues={{ sp_private_key: null }} + onSubmit={() => {}} + > + {(formik) => { + onForm(formik); + return ( + + ); + }} + + ); +} + +function picker(container: HTMLElement): HTMLInputElement { + const input = container.querySelector('input[type="file"]'); + if (!input) throw new Error("no file input"); + return input; +} + +describe("CredentialFileField as base64", () => { + it("holds the certificate's bytes as base64 text, and shows its name", async () => { + const user = setupUser(); + let form: FormikProps | null = null; + const { container } = render( + + initialValues={{ sp_private_key: null }} + onSubmit={() => {}} + > + {(formik) => { + form = formik; + return ( + + ); + }} + + ); + const picker = + container.querySelector('input[type="file"]'); + if (!picker) throw new Error("no file input"); + + // Bytes that are not valid text, as a .pfx's are. + await user.upload( + picker, + new File([new Uint8Array([0x30, 0x82, 0xff, 0x00, 0x01])], "app.pfx") + ); + + await waitFor(() => expect(form?.values.sp_private_key).toBe("MIL/AAE=")); + expect(screen.getByText("app.pfx")).toBeInTheDocument(); + }); + + it("keeps the latest pick when an earlier read lands after it", async () => { + const user = setupUser(); + const form: { current: FormikProps | null } = { + current: null, + }; + const { container } = renderField((formik) => (form.current = formik)); + let finishFirst: (value: string) => void = () => {}; + utils.fileToBase64 + .mockImplementationOnce( + () => new Promise((resolve) => (finishFirst = resolve)) + ) + .mockImplementationOnce(() => Promise.resolve("U0VDT05E")); + + await user.upload(picker(container), new File(["first"], "first.pfx")); + await user.upload(picker(container), new File(["second"], "second.pfx")); + await waitFor(() => + expect(form.current?.values.sp_private_key).toBe("U0VDT05E") + ); + finishFirst("RklSU1Q="); + + await waitFor(() => + expect(screen.getByText("second.pfx")).toBeInTheDocument() + ); + expect(form.current?.values.sp_private_key).toBe("U0VDT05E"); + expect(screen.queryByText("first.pfx")).not.toBeInTheDocument(); + }); + + it("shows an error, and holds no file, when a read fails", async () => { + const user = setupUser(); + const form: { current: FormikProps | null } = { + current: null, + }; + const { container } = renderField((formik) => (form.current = formik)); + utils.fileToBase64.mockImplementationOnce(() => + Promise.reject(new Error("unreadable")) + ); + + await user.upload(picker(container), new File(["x"], "broken.pfx")); + + expect(await screen.findByText("File selection error")).toBeInTheDocument(); + expect(form.current?.values.sp_private_key).toBeNull(); + expect(screen.queryByText("broken.pfx")).not.toBeInTheDocument(); + }); +}); diff --git a/web/src/lib/credentials/utils.ts b/web/src/lib/credentials/utils.ts index 584d8e504a..ffb93569b4 100644 --- a/web/src/lib/credentials/utils.ts +++ b/web/src/lib/credentials/utils.ts @@ -587,3 +587,18 @@ export function bindingCheckCredentialId( ? request.credential_id : null; } + +/** A file's bytes as base64 text, as an account stores a .pfx certificate. */ +export function fileToBase64(file: File): Promise { + return new Promise((resolve, reject) => { + const reader = new FileReader(); + // A data URL is "data:;base64,"; the content follows the + // first comma. + reader.onload = () => { + const result = typeof reader.result === "string" ? reader.result : ""; + resolve(result.slice(result.indexOf(",") + 1)); + }; + reader.onerror = () => reject(reader.error); + reader.readAsDataURL(file); + }); +} diff --git a/web/src/views/admin/connectors/AddConnectorPage/newAccount.test.ts b/web/src/views/admin/connectors/AddConnectorPage/newAccount.test.ts index b0c62fd386..6bac465ebc 100644 --- a/web/src/views/admin/connectors/AddConnectorPage/newAccount.test.ts +++ b/web/src/views/admin/connectors/AddConnectorPage/newAccount.test.ts @@ -53,3 +53,49 @@ test("a new account gets the realm, and the connector config does not", () => { ) ).toEqual({ content_type: "articles" }); }); + +test("a Microsoft account's certificate is typed in, as base64 text", () => { + const source = ValidSources.Sharepoint; + const spec = typedAccountSpec(source); + if (spec === null) throw new Error("SharePoint takes a typed account"); + const formValues: Record = { + [NEW_ACCOUNT_FIELD]: { + ...initialNewAccountValues(spec), + authentication_method: "certificate", + sp_client_id: "client", + sp_directory_id: "tenant", + sp_certificate_password: "secret", + sp_private_key: "MIIKHgIBAzCCCdQ=", + }, + }; + + const draft = typedDraft( + source, + newAccountSchema(spec, MESSAGES), + formValues + ); + + expect(draft?.credential_json).toMatchObject({ + authentication_method: "certificate", + sp_private_key: "MIIKHgIBAzCCCdQ=", + sp_certificate_password: "secret", + }); +}); + +test("a Microsoft certificate account is not ready without its file", () => { + const source = ValidSources.Sharepoint; + const spec = typedAccountSpec(source); + if (spec === null) throw new Error("SharePoint takes a typed account"); + + const draft = typedDraft(source, newAccountSchema(spec, MESSAGES), { + [NEW_ACCOUNT_FIELD]: { + ...initialNewAccountValues(spec), + authentication_method: "certificate", + sp_client_id: "client", + sp_directory_id: "tenant", + sp_certificate_password: "secret", + }, + }); + + expect(draft).toBeNull(); +}); diff --git a/web/src/views/admin/connectors/AddConnectorPage/newAccount.ts b/web/src/views/admin/connectors/AddConnectorPage/newAccount.ts index 8b647fc634..b58664530b 100644 --- a/web/src/views/admin/connectors/AddConnectorPage/newAccount.ts +++ b/web/src/views/admin/connectors/AddConnectorPage/newAccount.ts @@ -41,18 +41,13 @@ const OWN_ACCOUNT_SETUP: ReadonlySet = new Set([ /** * The source's credential spec when a new account is typed into the form; - * `null` when it is not: Google Drive and Gmail set accounts up their own - * way, and a file field (a key file) cannot be sent as typed values yet, so - * those still save an account through the account form. + * `null` when it is not, as Google Drive and Gmail set accounts up their own + * way. A file field (a .pfx certificate) is typed in too, as the file's + * base64 text, which is how the account stores it. */ export function typedAccountSpec(source: ValidSources): CredentialSpec | null { if (OWN_ACCOUNT_SETUP.has(source)) return null; - const spec = getCredentialSpec(source); - if (!spec) return null; - const hasFile = Object.values(spec.fields).some( - (field) => field.kind === "file" - ); - return hasFile ? null : spec; + return getCredentialSpec(source) ?? null; } /** diff --git a/web/src/views/admin/connectors/AddConnectorPage/sections/AuthenticationAccountSection.tsx b/web/src/views/admin/connectors/AddConnectorPage/sections/AuthenticationAccountSection.tsx index fc775858ae..4ceff93f9c 100644 --- a/web/src/views/admin/connectors/AddConnectorPage/sections/AuthenticationAccountSection.tsx +++ b/web/src/views/admin/connectors/AddConnectorPage/sections/AuthenticationAccountSection.tsx @@ -252,6 +252,8 @@ export default function AuthenticationAccountSection({ source={connector} spec={typedSpec} namePrefix={NEW_ACCOUNT_FIELD} + // Sent with the account's other values, as the draft holds it. + fileValues="base64" // Asked for above this section, first. exclude={realmKeys} authMethod={ diff --git a/web/tests/e2e/admin/connector/onedrive_connector_setup.spec.ts b/web/tests/e2e/admin/connector/onedrive_connector_setup.spec.ts index 32c412df41..523a97d82c 100644 --- a/web/tests/e2e/admin/connector/onedrive_connector_setup.spec.ts +++ b/web/tests/e2e/admin/connector/onedrive_connector_setup.spec.ts @@ -9,22 +9,20 @@ test.describe("OneDrive connector setup", () => { const setup = new OneDriveConnectorSetupPage(page); await setup.mockRoutes(); await setup.goto(); - await setup.createClientSecretCredential(); + await setup.fillClientSecretCredential(); await setup.runChecks(); await setup.selectSpecificScope("owner@example.com"); await setup.selectGeneralScope(); await setup.submitConnector("General OneDrive"); await setup.expectCreated(); - expect(setup.credentialRequests).toEqual([ - { - authenticationMethod: "client_secret", - clientId: "client-id", - directoryId: "directory-id", - hasClientSecret: true, - hasCertificatePassword: false, - }, - ]); + expect(setup.credentialRequest).toEqual({ + authenticationMethod: "client_secret", + clientId: "client-id", + directoryId: "directory-id", + hasClientSecret: true, + hasCertificatePassword: false, + }); const expectedConfig: OneDriveConfigRequest = { connector_specific_config: { users: [], @@ -37,23 +35,20 @@ test.describe("OneDrive connector setup", () => { const setup = new OneDriveConnectorSetupPage(page); await setup.mockRoutes(); await setup.goto(); - await setup.createCertificateCredential(); + await setup.fillCertificateCredential(); await setup.runChecks(); await setup.selectSpecificScope("owner@example.com"); await setup.submitConnector("Specific OneDrive"); await setup.expectCreated(); - expect(setup.credentialRequests).toEqual([ - { - authenticationMethod: "certificate", - clientId: "client-id", - directoryId: "directory-id", - hasClientSecret: false, - hasCertificatePassword: true, - privateKeyField: "onedrive_private_key", - privateKeyType: "onedrive_pfx_file", - }, - ]); + expect(setup.credentialRequest).toEqual({ + authenticationMethod: "certificate", + clientId: "client-id", + directoryId: "directory-id", + hasClientSecret: false, + hasCertificatePassword: true, + privateKey: Buffer.from("test-pfx").toString("base64"), + }); const expectedConfig: OneDriveConfigRequest = { connector_specific_config: { users: ["owner@example.com"], @@ -67,7 +62,7 @@ test.describe("OneDrive connector setup", () => { const setup = new OneDriveConnectorSetupPage(page); await setup.mockRoutes(); await setup.goto(); - await setup.createClientSecretCredential(); + await setup.fillClientSecretCredential(); await setup.runChecks(); await setup.selectSpecificScope(); await setup.submitInvalidConnector( diff --git a/web/tests/e2e/pages/OneDriveConnectorSetupPage.ts b/web/tests/e2e/pages/OneDriveConnectorSetupPage.ts index 9f3212d6f8..4065e4232f 100644 --- a/web/tests/e2e/pages/OneDriveConnectorSetupPage.ts +++ b/web/tests/e2e/pages/OneDriveConnectorSetupPage.ts @@ -1,4 +1,4 @@ -import { expect, type Page, type Request } from "@playwright/test"; +import { expect, type Page } from "@playwright/test"; import { mockPassingConnectorChecks, runConnectorChecks, @@ -16,22 +16,21 @@ export interface OneDriveCredentialRequest { directoryId: string; hasClientSecret: boolean; hasCertificatePassword: boolean; - privateKeyField?: string; - privateKeyType?: string; + /** The certificate, as the base64 text the account stores. */ + privateKey?: string; } function isRecord(value: unknown): value is Record { return typeof value === "object" && value !== null; } -function parseCredentialJson(value: unknown): OneDriveCredentialRequest { - if (!isRecord(value)) { - throw new Error("Credential request must be an object"); - } +/** The draft account a check run sends, or `null` for a saved account. */ +function parseDraftCredential( + value: unknown +): OneDriveCredentialRequest | null { + if (!isRecord(value) || !isRecord(value.credential_json)) return null; const credentialJson = value.credential_json; - if (!isRecord(credentialJson)) { - throw new Error("Credential JSON must be an object"); - } + const privateKey = credentialJson.onedrive_private_key; return { authenticationMethod: String(credentialJson.authentication_method ?? ""), clientId: String(credentialJson.onedrive_client_id ?? ""), @@ -42,39 +41,13 @@ function parseCredentialJson(value: unknown): OneDriveCredentialRequest { hasCertificatePassword: typeof credentialJson.onedrive_certificate_password === "string" && credentialJson.onedrive_certificate_password.length > 0, - }; -} - -function getMultipartField(body: string, fieldName: string): string { - const fieldMarker = `name="${fieldName}"`; - const fieldStart = body.indexOf(fieldMarker); - if (fieldStart < 0) { - return ""; - } - const valueStart = body.indexOf("\r\n\r\n", fieldStart); - if (valueStart < 0) { - return ""; - } - const valueEnd = body.indexOf("\r\n--", valueStart); - return body.slice(valueStart + 4, valueEnd < 0 ? undefined : valueEnd); -} - -function parsePrivateKeyCredential( - request: Request -): OneDriveCredentialRequest { - const body = request.postData() ?? ""; - const credential = parseCredentialJson({ - credential_json: JSON.parse(getMultipartField(body, "credential_json")), - }); - return { - ...credential, - privateKeyField: getMultipartField(body, "field_key"), - privateKeyType: getMultipartField(body, "type_definition_key"), + ...(typeof privateKey === "string" ? { privateKey } : {}), }; } export class OneDriveConnectorSetupPage { - readonly credentialRequests: OneDriveCredentialRequest[] = []; + /** The draft account of the latest check run. */ + credentialRequest: OneDriveCredentialRequest | null = null; readonly connectorRequests: OneDriveConfigRequest[] = []; constructor(private readonly page: Page) {} @@ -93,16 +66,6 @@ export class OneDriveConnectorSetupPage { }, }) ); - await this.page.route("**/api/manage/credential/private-key", (route) => { - this.credentialRequests.push(parsePrivateKeyCredential(route.request())); - return route.fulfill({ json: this.credentialResponse(42) }); - }); - await this.page.route("**/api/manage/credential", (route) => { - this.credentialRequests.push( - parseCredentialJson(route.request().postDataJSON()) - ); - return route.fulfill({ json: this.credentialResponse(41) }); - }); // Create makes the connector and pairs it in one request. await this.page.route( "**/api/manage/admin/connector-with-credential", @@ -139,6 +102,16 @@ export class OneDriveConnectorSetupPage { } ); await mockPassingConnectorChecks(this.page); + // A typed account travels with the check run, which saves it as a draft. + // Registered last, so it sees the run first and hands it to the mock. + await this.page.route( + "**/api/manage/admin/connector-checks/runs", + (route) => { + const draft = parseDraftCredential(route.request().postDataJSON()); + if (draft) this.credentialRequest = draft; + return route.fallback(); + } + ); } async goto() { @@ -148,14 +121,13 @@ export class OneDriveConnectorSetupPage { ).toBeVisible(); } - async createClientSecretCredential() { + async fillClientSecretCredential() { await this.openCredentialForm(); await this.fillCredentialIdentity(); await this.page.getByTestId("onedrive_client_secret").fill("secret-value"); - await this.submitCredential(); } - async createCertificateCredential() { + async fillCertificateCredential() { await this.openCredentialForm(); await this.page.getByRole("tab", { name: "Certificate" }).click(); await this.fillCredentialIdentity(); @@ -167,7 +139,6 @@ export class OneDriveConnectorSetupPage { mimeType: "application/x-pkcs12", buffer: Buffer.from("test-pfx"), }); - await this.submitCredential(); } /** Run the connector checks and wait for the configuration to unlock. */ @@ -216,11 +187,6 @@ export class OneDriveConnectorSetupPage { await expect(this.page).toHaveURL(/\/admin\/indexing-status/); } - /** The creation form, once its card is expanded. */ - private get credentialForm() { - return this.page.getByTestId("credential-form"); - } - /** * Opens the new-account card. It starts open when there is no saved * account, so it is clicked only while closed; retried in case its starting @@ -245,43 +211,4 @@ export class OneDriveConnectorSetupPage { await this.page.getByTestId("onedrive_client_id").fill("client-id"); await this.page.getByTestId("onedrive_directory_id").fill("directory-id"); } - - private async submitCredential() { - const responsePromise = this.page.waitForResponse( - (response) => - response.request().method() === "POST" && - [ - "/api/manage/credential", - "/api/manage/credential/private-key", - ].includes(new URL(response.url()).pathname) - ); - await this.credentialForm - .getByRole("button", { name: "Create", exact: true }) - .click(); - const response = await responsePromise; - expect(response.ok()).toBeTruthy(); - // The card folds itself away on success. The fold is hidden from the - // accessibility tree, so the form's own button stops resolving. - await expect( - this.page.getByRole("button", { name: "Create", exact: true }) - ).toBeHidden(); - } - - private credentialResponse(id: number) { - const timestamp = new Date(0).toISOString(); - return { - id, - credential: { - id, - credential_json: {}, - admin_public: true, - source: "onedrive", - name: "OneDrive test credential", - user_id: null, - user_email: null, - time_created: timestamp, - time_updated: timestamp, - }, - }; - } }