diff --git a/src/Common/ErrorHandlingUtils.ts b/src/Common/ErrorHandlingUtils.ts index 6996f19cd..9755c8a63 100644 --- a/src/Common/ErrorHandlingUtils.ts +++ b/src/Common/ErrorHandlingUtils.ts @@ -42,8 +42,8 @@ export const handleError = ( } }; -export const getErrorMessage = (error: string | Error = ""): string => { - let errorMessage = typeof error === "string" ? error : error.message; +export const getErrorMessage = (error: unknown = ""): string => { + let errorMessage = typeof error === "string" ? error : (error as Error)?.message; if (!errorMessage) { errorMessage = stringifyError(error); } diff --git a/src/HostedExplorer.tsx b/src/HostedExplorer.tsx index 982d37d4b..5b4ecb027 100644 --- a/src/HostedExplorer.tsx +++ b/src/HostedExplorer.tsx @@ -1,5 +1,6 @@ import { initializeIcons } from "@fluentui/react"; import { useBoolean } from "@fluentui/react-hooks"; +import { getErrorMessage } from "Common/ErrorHandlingUtils"; import { AadAuthorizationFailure } from "Platform/Hosted/Components/AadAuthorizationFailure"; import * as React from "react"; import { render } from "react-dom"; @@ -47,6 +48,7 @@ const App: React.FunctionComponent = () => { const [databaseAccount, setDatabaseAccount] = React.useState(); const [authType, setAuthType] = React.useState(encryptedToken ? AuthType.EncryptedToken : undefined); const [connectionString, setConnectionString] = React.useState(); + const [errorMessage, setErrorMessage] = React.useState(); // For SQL/Tables/Gremlin connection-string login, the account metadata is derived client-side from the // connection string instead of the Portal Backend, so there is no encrypted token. const [directLoginMetadata, setDirectLoginMetadata] = React.useState(); @@ -59,6 +61,7 @@ const App: React.FunctionComponent = () => { if (!connStr || authType) { return; } + setErrorMessage(undefined); setConnectionString(connStr); if (isResourceTokenConnectionString(connStr)) { setAuthType(AuthType.ResourceToken); @@ -75,10 +78,12 @@ const App: React.FunctionComponent = () => { setAuthType(AuthType.ConnectionString); }) .catch((error) => { + const message = getErrorMessage(error); logError( - `Failed to connect with connection string: ${error}`, + `Failed to connect with connection string: ${message}`, "HostedExplorer/connectWithConnectionString", ); + setErrorMessage(message); }); return; } @@ -89,7 +94,12 @@ const App: React.FunctionComponent = () => { setAuthType(AuthType.ConnectionString); }) .catch((error) => { - logError(`Failed to connect with connection string: ${error}`, "HostedExplorer/connectWithConnectionString"); + const message = getErrorMessage(error); + logError( + `Failed to connect with connection string: ${message}`, + "HostedExplorer/connectWithConnectionString", + ); + setErrorMessage(message); }); }, [authType], @@ -226,7 +236,16 @@ const App: React.FunctionComponent = () => { )} {!isLoggedIn && !encryptedTokenMetadata && ( )} {isLoggedIn && authFailure && } diff --git a/src/Localization/en/Resources.json b/src/Localization/en/Resources.json index 9a3c41324..372bb6512 100644 --- a/src/Localization/en/Resources.json +++ b/src/Localization/en/Resources.json @@ -1177,8 +1177,7 @@ }, "connectExplorer": { "errors": { - "connectFailed": "Failed to connect using the provided connection string. Please verify it is correct and try again.", - "connectivityUnreachable": "Unable to connect to the account. Please verify the connection string is correct and that the account is reachable." + "connectFailed": "Failed to connect using the provided connection string. Please verify it is correct and try again." } } } diff --git a/src/Platform/Hosted/Components/ConnectExplorer.test.tsx b/src/Platform/Hosted/Components/ConnectExplorer.test.tsx index 60ce62c37..10ca86e72 100644 --- a/src/Platform/Hosted/Components/ConnectExplorer.test.tsx +++ b/src/Platform/Hosted/Components/ConnectExplorer.test.tsx @@ -1,10 +1,18 @@ jest.mock("../../../hooks/useDirectories"); +jest.mock("Common/CosmosClient", () => ({ + client: jest.fn(), +})); +jest.mock("Common/Logger", () => ({ + logWarning: jest.fn(), +})); import "@testing-library/jest-dom"; import { fireEvent, render, screen } from "@testing-library/react"; +import { client } from "Common/CosmosClient"; import { extractFeatures } from "Platform/Hosted/extractFeatures"; import { updateUserContext, userContext } from "UserContext"; import React from "react"; -import { ConnectExplorer } from "./ConnectExplorer"; +import { AccessInputMetadata } from "../../../Contracts/DataModels"; +import { ConnectExplorer, validateDirectConnectionStringConnectivity } from "./ConnectExplorer"; it("shows the connect form", () => { const connectionString = "fakeConnectionString"; @@ -13,10 +21,19 @@ it("shows the connect form", () => { const setEncryptedToken = jest.fn(); const setAuthType = jest.fn(); const setDirectLoginMetadata = jest.fn(); + const setErrorMessage = jest.fn(); render( , ); expect(screen.queryByPlaceholderText("Please enter a connection string")).toBeNull(); @@ -31,6 +48,7 @@ it("hides the connection string link when feature.disableConnectionStringLogin i const setEncryptedToken = jest.fn(); const setAuthType = jest.fn(); const setDirectLoginMetadata = jest.fn(); + const setErrorMessage = jest.fn(); const oldFeatures = userContext.features; const params = new URLSearchParams({ @@ -42,10 +60,57 @@ it("hides the connection string link when feature.disableConnectionStringLogin i render( , ); expect(screen.queryByPlaceholderText("Connect to your account with connection string")).toBeNull(); updateUserContext({ features: oldFeatures }); }); + +describe("validateDirectConnectionStringConnectivity", () => { + const connectionString = "AccountEndpoint=https://test.documents.azure.com:443/;AccountKey=fakeKey==;"; + const metadata = { documentEndpoint: "https://test.documents.azure.com:443/" } as AccessInputMetadata; + + const mockGetDatabaseAccount = (getDatabaseAccount: () => Promise) => { + (client as jest.Mock).mockReturnValue({ getDatabaseAccount }); + }; + + it.each([401, 403])("blocks the login and surfaces the error returned for a %i", async (statusCode) => { + // The Cosmos SDK reports the HTTP status on `ErrorResponse.code`. + const authorizationError = Object.assign(new Error("The input authorization token is invalid"), { + code: statusCode, + }); + mockGetDatabaseAccount(() => Promise.reject(authorizationError)); + + await expect(validateDirectConnectionStringConnectivity(connectionString, metadata)).rejects.toBe( + authorizationError, + ); + }); + + it.each([429, 503])("does not block the login for a %i", async (statusCode) => { + mockGetDatabaseAccount(() => Promise.reject(Object.assign(new Error("Service unavailable"), { code: statusCode }))); + + await expect(validateDirectConnectionStringConnectivity(connectionString, metadata)).resolves.toBeUndefined(); + }); + + it("does not block the login when the account is unreachable", async () => { + mockGetDatabaseAccount(() => Promise.reject(new Error("Failed to fetch"))); + + await expect(validateDirectConnectionStringConnectivity(connectionString, metadata)).resolves.toBeUndefined(); + }); + + it("allows the login when the account can be read", async () => { + mockGetDatabaseAccount(() => Promise.resolve({})); + + await expect(validateDirectConnectionStringConnectivity(connectionString, metadata)).resolves.toBeUndefined(); + }); +}); diff --git a/src/Platform/Hosted/Components/ConnectExplorer.tsx b/src/Platform/Hosted/Components/ConnectExplorer.tsx index a0b65ad58..f83b18e8e 100644 --- a/src/Platform/Hosted/Components/ConnectExplorer.tsx +++ b/src/Platform/Hosted/Components/ConnectExplorer.tsx @@ -1,5 +1,7 @@ import { useBoolean } from "@fluentui/react-hooks"; import { client } from "Common/CosmosClient"; +import { getErrorMessage } from "Common/ErrorHandlingUtils"; +import { logWarning } from "Common/Logger"; import { Keys, t } from "Localization"; import { updateUserContext, userContext } from "UserContext"; import * as React from "react"; @@ -9,6 +11,7 @@ import { AuthType } from "../../../AuthType"; import { HttpHeaders } from "../../../Common/Constants"; import { configContext } from "../../../ConfigContext"; import { AccessInputMetadata } from "../../../Contracts/DataModels"; +import { isAuthorizationError } from "../../../Utils/AuthorizationUtils"; import { parseConnectionString } from "../Helpers/ConnectionStringParser"; import { isResourceTokenConnectionString } from "../Helpers/ResourceTokenUtils"; import { extractMasterKeyfromConnectionString, isDirectConnectionStringLoginApi } from "../HostedUtils"; @@ -20,8 +23,15 @@ interface Props { setConnectionString: (connectionString: string) => void; setAuthType: (authType: AuthType) => void; setDirectLoginMetadata: (metadata: AccessInputMetadata) => void; + errorMessage?: string; + setErrorMessage: (message: string) => void; } +// Turns a failed Portal Backend response into an error that carries the message returned by the service +// and the HTTP status code, so the caller can tell an authorization failure from anything else. +const errorFromResponse = async (response: Response): Promise => + Object.assign(new Error((await response.text()) || response.statusText), { statusCode: response.status }); + export const fetchEncryptedToken = async (connectionString: string): Promise => { const headers = new Headers(); headers.append(HttpHeaders.connectionString, connectionString); @@ -29,7 +39,7 @@ export const fetchEncryptedToken = async (connectionString: string): Promise => { const masterKey = extractMasterKeyfromConnectionString(connectionString); if (!metadata?.documentEndpoint || !masterKey) { - throw new Error(t(Keys.connectExplorer.errors.connectivityUnreachable)); + throw new Error(t(Keys.connectExplorer.errors.connectFailed)); } updateUserContext({ @@ -68,8 +81,15 @@ export const validateDirectConnectionStringConnectivity = async ( try { await client().getDatabaseAccount(); - } catch { - throw new Error(t(Keys.connectExplorer.errors.connectivityUnreachable)); + } catch (error) { + if (isAuthorizationError(error)) { + throw error; + } + + logWarning( + `Could not verify the connection string against the account: ${getErrorMessage(error)}`, + "ConnectExplorer/validateDirectConnectionStringConnectivity", + ); } }; @@ -80,9 +100,10 @@ export const ConnectExplorer: React.FunctionComponent = ({ connectionString, setConnectionString, setDirectLoginMetadata, + errorMessage, + setErrorMessage, }: Props) => { const [isFormVisible, { setTrue: showForm }] = useBoolean(false); - const [errorMessage, setErrorMessage] = React.useState(""); const enableConnectionStringLogin = !userContext.features.disableConnectionStringLogin; return ( @@ -100,10 +121,15 @@ export const ConnectExplorer: React.FunctionComponent = ({ event.preventDefault(); setErrorMessage(""); - if (await isAccountRestrictedForConnectionStringLogin(connectionString)) { - setErrorMessage( - "This account has been blocked from connection-string login. Please go to cosmos.azure.com/aad for AAD based login.", - ); + try { + if (await isAccountRestrictedForConnectionStringLogin(connectionString)) { + setErrorMessage( + "This account has been blocked from connection-string login. Please go to cosmos.azure.com/aad for AAD based login.", + ); + return; + } + } catch (error) { + setErrorMessage(getErrorMessage(error)); return; } @@ -121,7 +147,7 @@ export const ConnectExplorer: React.FunctionComponent = ({ setDirectLoginMetadata(metadata); setAuthType(AuthType.ConnectionString); } catch (error) { - setErrorMessage(error.message); + setErrorMessage(getErrorMessage(error)); } return; } @@ -131,7 +157,7 @@ export const ConnectExplorer: React.FunctionComponent = ({ setEncryptedToken(encryptedToken); setAuthType(AuthType.ConnectionString); } catch (error) { - setErrorMessage(t(Keys.connectExplorer.errors.connectivityUnreachable)); + setErrorMessage(getErrorMessage(error)); } }} > @@ -147,7 +173,7 @@ export const ConnectExplorer: React.FunctionComponent = ({ setConnectionString(event.target.value); }} /> - {errorMessage.length > 0 && ( + {errorMessage && ( Error notification {errorMessage} @@ -169,6 +195,12 @@ export const ConnectExplorer: React.FunctionComponent = ({ Connect to your account with connection string

)} + {errorMessage && ( + + Error notification + {errorMessage} + + )} )} diff --git a/src/Platform/Hosted/ConnectScreen.less b/src/Platform/Hosted/ConnectScreen.less index d4123f23f..f1fb81241 100644 --- a/src/Platform/Hosted/ConnectScreen.less +++ b/src/Platform/Hosted/ConnectScreen.less @@ -65,27 +65,35 @@ visibility: visible; } .connectExplorerContainer .connectExplorer .connectExplorerContent .errorDetailsInfoTooltip .errorDetails { - bottom: 24px; - width: 165px; + top: 50%; + transform: translateY(-50%); + left: 100%; + margin-left: 10px; + width: 320px; + max-height: 240px; + overflow-y: auto; visibility: hidden; background-color: #393939; color: #ffffff; position: absolute; z-index: 1; - left: -10px; padding: 6px; + text-align: left; + white-space: normal; + overflow-wrap: break-word; } -.connectExplorerContainer .connectExplorer .connectExplorerContent .errorDetailsInfoTooltip .errorDetails:after { - border-width: 10px 10px 0px 10px; - bottom: -8px; +.connectExplorerContainer .connectExplorer .connectExplorerContent .errorDetailsInfoTooltip:hover:after { + border-width: 8px 8px 8px 0px; content: ""; position: absolute; - right: 100%; + top: 50%; + transform: translateY(-50%); + left: 100%; + margin-left: 2px; border-style: solid; - left: 12px; width: 0; height: 0; - border-color: #3b3b3b transparent; + border-color: transparent #393939; } .connectExplorerContainer .connectExplorer .connectExplorerContent .errorDetailsInfoTooltip .errorImg { height: 14px; diff --git a/src/Utils/AuthorizationUtils.test.ts b/src/Utils/AuthorizationUtils.test.ts index fd361ac22..960516520 100644 --- a/src/Utils/AuthorizationUtils.test.ts +++ b/src/Utils/AuthorizationUtils.test.ts @@ -203,4 +203,38 @@ describe("AuthorizationUtils", () => { expect(AuthorizationUtils.getRedirectBridgeUrl()).toBe("https://cosmos.azure.com/redirectBridge.html"); }); }); + + describe("isAuthorizationError()", () => { + it.each([401, 403])("treats a Cosmos client `code` of %i as an authorization failure", (statusCode) => { + expect( + AuthorizationUtils.isAuthorizationError(Object.assign(new Error("Unauthorized"), { code: statusCode })), + ).toBe(true); + }); + + it.each([401, 403])("treats a `statusCode` of %i as an authorization failure", (statusCode) => { + expect(AuthorizationUtils.isAuthorizationError(Object.assign(new Error("Unauthorized"), { statusCode }))).toBe( + true, + ); + }); + + it("handles a status reported as a string", () => { + expect(AuthorizationUtils.isAuthorizationError({ code: "403" })).toBe(true); + }); + + it.each([404, 429, 500, 503])("does not treat %i as an authorization failure", (statusCode) => { + expect(AuthorizationUtils.isAuthorizationError({ code: statusCode })).toBe(false); + }); + + it("does not treat a non numeric code as an authorization failure", () => { + expect(AuthorizationUtils.isAuthorizationError({ code: "ENOTFOUND" })).toBe(false); + }); + + it("does not treat a network failure as an authorization failure", () => { + expect(AuthorizationUtils.isAuthorizationError(new Error("Failed to fetch"))).toBe(false); + }); + + it("handles a missing error", () => { + expect(AuthorizationUtils.isAuthorizationError(undefined)).toBe(false); + }); + }); }); diff --git a/src/Utils/AuthorizationUtils.ts b/src/Utils/AuthorizationUtils.ts index 0938c6f33..2d3e92edf 100644 --- a/src/Utils/AuthorizationUtils.ts +++ b/src/Utils/AuthorizationUtils.ts @@ -254,3 +254,14 @@ export function isCloudShellEntraAuthEnabled(userContext: UserContext): boolean useDataplaneRbacAuthorization(userContext) || userContext.databaseAccount?.properties?.disableLocalAuth === true ); } + +/** + * True when an error is an authorization failure (401 Unauthorized or 403 Forbidden). The Cosmos client + * reports the HTTP status on `ErrorResponse.code`; errors raised from a `fetch` response carry it on + * `statusCode`. + */ +export function isAuthorizationError(error: unknown): boolean { + const { code, statusCode } = (error ?? {}) as { code?: unknown; statusCode?: unknown }; + const status = Number(statusCode ?? code); + return status === Constants.HttpStatusCodes.Unauthorized || status === Constants.HttpStatusCodes.Forbidden; +} diff --git a/test/sql/connectionStringLogin.spec.ts b/test/sql/connectionStringLogin.spec.ts index 880e84cb8..89e3ed914 100644 --- a/test/sql/connectionStringLogin.spec.ts +++ b/test/sql/connectionStringLogin.spec.ts @@ -1,4 +1,4 @@ -import { expect, test } from "@playwright/test"; +import { Page, expect, test } from "@playwright/test"; import { CosmosDBManagementClient } from "@azure/arm-cosmosdb"; import { CosmosClient, Database } from "@azure/cosmos"; @@ -18,8 +18,20 @@ const databaseId = generateUniqueName("db"); const containerId = "testcontainer"; const documentId = "testdoc1"; +async function loginWithConnectionString(page: Page, connectionString: string): Promise { + await page.goto("https://localhost:1234/hostedExplorer.html"); + const switchConnectionLink = page.getByTestId("Link:SwitchConnectionType"); + await switchConnectionLink.waitFor(); + await switchConnectionLink.click(); + await page.getByPlaceholder("Please enter a connection string").fill(connectionString); + await page.getByRole("button", { name: "Connect" }).click(); +} + test.describe("SQL account using connection string login", () => { let database: Database = null!; + let documentEndpoint: string = null!; + // SQL signs data-plane requests client-side with the account key, so no encrypted token is issued. + let connectionString: string = null!; test.beforeAll("Seed Test Database", async () => { const credentials = getAzureCLICredentials(); @@ -27,8 +39,10 @@ test.describe("SQL account using connection string login", () => { const accountName = getAccountName(TestAccount.SQL, TestAuthType.ConnectionString); const account = await armClient.databaseAccounts.get(resourceGroupName, accountName); const keys = await armClient.databaseAccounts.listKeys(resourceGroupName, accountName); + documentEndpoint = account.documentEndpoint!; + connectionString = `AccountEndpoint=${documentEndpoint};AccountKey=${keys.primaryMasterKey};`; - const client = new CosmosClient({ endpoint: account.documentEndpoint!, key: keys.primaryMasterKey }); + const client = new CosmosClient({ endpoint: documentEndpoint, key: keys.primaryMasterKey }); database = (await client.databases.createIfNotExists({ id: databaseId })).database; const { container } = await database.containers.createIfNotExists({ id: containerId, @@ -42,21 +56,7 @@ test.describe("SQL account using connection string login", () => { }); test("reads a document after connection string login", async ({ page }) => { - const credentials = getAzureCLICredentials(); - const armClient = new CosmosDBManagementClient(credentials, subscriptionId); - const accountName = getAccountName(TestAccount.SQL, TestAuthType.ConnectionString); - const account = await armClient.databaseAccounts.get(resourceGroupName, accountName); - const keys = await armClient.databaseAccounts.listKeys(resourceGroupName, accountName); - - // SQL signs data-plane requests client-side with the account key, so no encrypted token is issued. - const connectionString = `AccountEndpoint=${account.documentEndpoint};AccountKey=${keys.primaryMasterKey};`; - - await page.goto("https://localhost:1234/hostedExplorer.html"); - const switchConnectionLink = page.getByTestId("Link:SwitchConnectionType"); - await switchConnectionLink.waitFor(); - await switchConnectionLink.click(); - await page.getByPlaceholder("Please enter a connection string").fill(connectionString); - await page.getByRole("button", { name: "Connect" }).click(); + await loginWithConnectionString(page, connectionString); const explorer = await DataExplorer.waitForExplorer(page); const collectionNode = await explorer.waitForContainerNode(databaseId, containerId); @@ -84,26 +84,52 @@ test.describe("SQL account using connection string login", () => { }); test("shows an error when the connection string has the wrong account key", async ({ page }) => { - const credentials = getAzureCLICredentials(); - const armClient = new CosmosDBManagementClient(credentials, subscriptionId); - const accountName = getAccountName(TestAccount.SQL, TestAuthType.ConnectionString); - const account = await armClient.databaseAccounts.get(resourceGroupName, accountName); - // A well-formed but incorrect base64 account key (88-char, 64-byte): the endpoint is valid, so the // Cosmos client reaches the account but the data-plane request is rejected with 401 Unauthorized. const wrongKey = "A".repeat(86) + "=="; - const connectionString = `AccountEndpoint=${account.documentEndpoint};AccountKey=${wrongKey};`; + await loginWithConnectionString(page, `AccountEndpoint=${documentEndpoint};AccountKey=${wrongKey};`); - await page.goto("https://localhost:1234/hostedExplorer.html"); - const switchConnectionLink = page.getByTestId("Link:SwitchConnectionType"); - await switchConnectionLink.waitFor(); - await switchConnectionLink.click(); - await page.getByPlaceholder("Please enter a connection string").fill(connectionString); - await page.getByRole("button", { name: "Connect" }).click(); - - // The connect form stays visible and surfaces the connectivity error instead of opening the explorer. - await expect(page.locator(".errorDetails")).toContainText("Unable to connect to the account", { + // The connect form stays visible and surfaces the 401 returned by the account instead of opening Data Explorer. + await expect(page.locator(".errorDetails")).toContainText("The wrong key is being used", { timeout: ONE_MINUTE_MS, }); }); + + test("opens the explorer when the connectivity check fails with a non-authorization error", async ({ page }) => { + // The pre-login connectivity check calls getDatabaseAccount from the top-level hosted explorer frame, + // which the dev server forwards from the proxy root. Failing only those requests with a 500 simulates a + // transient or internal service failure while leaving the requests Data Explorer makes from the iframe + // untouched, so the explorer still talks to the real account once the user is in. + const failedProxyTargets: string[] = []; + await page.route( + (url) => url.pathname === "/proxy", + async (route) => { + const request = route.request(); + if (request.frame() !== page.mainFrame()) { + await route.continue(); + return; + } + + failedProxyTargets.push(request.headers()["x-ms-proxy-target"]); + await route.fulfill({ + status: 500, + contentType: "application/json", + body: JSON.stringify({ code: "InternalServerError", message: "The service is temporarily unavailable." }), + }); + }, + ); + + // The account key is correct, so nothing about this login is a credential problem. + await loginWithConnectionString(page, connectionString); + + // The failed check does not block login: the explorer opens and reads the account through the data plane. + const explorer = await DataExplorer.waitForExplorer(page); + const collectionNode = await explorer.waitForContainerNode(databaseId, containerId); + await expect(collectionNode.element).toBeAttached(); + + // The connectivity check ran against the endpoint from the connection string and did fail, and the + // transient failure was never surfaced to the user as a login error. + expect(failedProxyTargets).toContain(documentEndpoint); + await expect(page.locator(".errorDetails")).toHaveCount(0); + }); });