diff --git a/src/HostedExplorer.test.tsx b/src/HostedExplorer.test.tsx index 9ee38cef5..71b4df9c0 100644 --- a/src/HostedExplorer.test.tsx +++ b/src/HostedExplorer.test.tsx @@ -28,16 +28,10 @@ import React from "react"; import { useAADAuth } from "./hooks/useAADAuth"; import { useConfig } from "./hooks/useConfig"; import { App } from "./HostedExplorer"; -import { - ConnectExplorer, - fetchEncryptedToken, - validateDirectConnectionStringConnectivity, -} from "./Platform/Hosted/Components/ConnectExplorer"; +import { ConnectExplorer, fetchEncryptedToken } from "./Platform/Hosted/Components/ConnectExplorer"; import { fetchAccessData } from "./Platform/Hosted/Helpers/PortalAccessData"; const mockFetchEncryptedToken = fetchEncryptedToken as jest.MockedFunction; -const mockValidateDirectConnectionStringConnectivity = - validateDirectConnectionStringConnectivity as jest.MockedFunction; (ConnectExplorer as jest.Mock).mockImplementation(() =>
); @@ -62,7 +56,6 @@ beforeEach(() => { (useConfig as jest.Mock).mockReturnValue({}); (fetchAccessData as jest.Mock).mockResolvedValue(undefined); mockFetchEncryptedToken.mockResolvedValue("encrypted-token"); - mockValidateDirectConnectionStringConnectivity.mockResolvedValue(undefined); }); const dispatchPostMessage = (data: unknown, origin: string) => { @@ -89,7 +82,6 @@ describe("HostedExplorer tryCosmosDB postMessage handler", () => { }); expect(mockFetchEncryptedToken).not.toHaveBeenCalled(); - expect(mockValidateDirectConnectionStringConnectivity).toHaveBeenCalled(); }); it("accepts a valid Mongo connection string from an allowed origin", async () => { @@ -138,7 +130,6 @@ describe("HostedExplorer tryCosmosDB postMessage handler", () => { }); expect(mockFetchEncryptedToken).not.toHaveBeenCalled(); - expect(mockValidateDirectConnectionStringConnectivity).toHaveBeenCalled(); }); it("accepts a valid Gremlin connection string from an allowed origin", async () => { @@ -155,26 +146,6 @@ describe("HostedExplorer tryCosmosDB postMessage handler", () => { }); expect(mockFetchEncryptedToken).not.toHaveBeenCalled(); - expect(mockValidateDirectConnectionStringConnectivity).toHaveBeenCalled(); - }); - - it("does not open Data Explorer when the Cosmos client cannot connect", async () => { - mockValidateDirectConnectionStringConnectivity.mockRejectedValue(new Error("Unable to connect to the account.")); - const { container } = render(); - - const validConnStr = `AccountEndpoint=https://${FAKE_ACCOUNT_NAME}.documents.azure.com:443/;AccountKey=${FAKE_KEY};`; - - await act(async () => { - dispatchPostMessage( - { type: "tryCosmosDBConnectionString", connectionString: validConnStr }, - "https://cosmos.azure.com", - ); - await Promise.resolve(); - }); - - expect(mockValidateDirectConnectionStringConnectivity).toHaveBeenCalled(); - expect(mockFetchEncryptedToken).not.toHaveBeenCalled(); - expect(container.querySelector('[data-test="DataExplorerFrame"]')).toBeNull(); }); it("rejects messages from a disallowed origin", async () => { diff --git a/src/HostedExplorer.tsx b/src/HostedExplorer.tsx index 79cc39b0c..5f17348db 100644 --- a/src/HostedExplorer.tsx +++ b/src/HostedExplorer.tsx @@ -12,11 +12,7 @@ import { AccessInputMetadata, DatabaseAccount } from "./Contracts/DataModels"; import "./Explorer/Menus/NavBar/MeControlComponent.less"; import { HostedExplorerChildFrame } from "./HostedExplorerChildFrame"; import { AccountSwitcher } from "./Platform/Hosted/Components/AccountSwitcher"; -import { - ConnectExplorer, - fetchEncryptedToken, - validateDirectConnectionStringConnectivity, -} from "./Platform/Hosted/Components/ConnectExplorer"; +import { ConnectExplorer, fetchEncryptedToken } from "./Platform/Hosted/Components/ConnectExplorer"; import { DirectoryPickerPanel } from "./Platform/Hosted/Components/DirectoryPickerPanel"; import { FeedbackCommandButton } from "./Platform/Hosted/Components/FeedbackCommandButton"; import { MeControl } from "./Platform/Hosted/Components/MeControl"; @@ -76,19 +72,8 @@ const App: React.FunctionComponent = () => { if (metadata && isDirectConnectionStringLoginApi(metadata.apiKind)) { // SQL, Tables, and Gremlin sign data-plane requests client-side with the account key, so we skip // the Portal Backend proxy and use the metadata derived from the connection string directly. - validateDirectConnectionStringConnectivity(connStr, metadata) - .then(() => { - setAccountMetadata(metadata); - setAuthType(AuthType.ConnectionString); - }) - .catch((error) => { - const message = getErrorMessage(error); - logError( - `Failed to connect with connection string: ${message}`, - "HostedExplorer/connectWithConnectionString", - ); - setErrorMessage(message); - }); + setAccountMetadata(metadata); + setAuthType(AuthType.ConnectionString); return; } diff --git a/src/Localization/en/Resources.json b/src/Localization/en/Resources.json index 2844e2644..f0301eb47 100644 --- a/src/Localization/en/Resources.json +++ b/src/Localization/en/Resources.json @@ -1174,10 +1174,5 @@ "retryConsoleMessage": "Data transfer job \"{{jobName}}\" in progress, total count: {{totalCount}}, processed count: {{processedCount}}" } } - }, - "connectExplorer": { - "errors": { - "connectFailed": "Failed to connect using the provided connection string. Please verify it is correct and that the account is reachable, then try again." - } } } diff --git a/src/Platform/Hosted/Components/ConnectExplorer.test.tsx b/src/Platform/Hosted/Components/ConnectExplorer.test.tsx index bea5992e1..3c8c40cb7 100644 --- a/src/Platform/Hosted/Components/ConnectExplorer.test.tsx +++ b/src/Platform/Hosted/Components/ConnectExplorer.test.tsx @@ -1,18 +1,10 @@ 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 { AccessInputMetadata } from "../../../Contracts/DataModels"; -import { ConnectExplorer, validateDirectConnectionStringConnectivity } from "./ConnectExplorer"; +import { ConnectExplorer } from "./ConnectExplorer"; it("shows the connect form", () => { const connectionString = "fakeConnectionString"; @@ -75,42 +67,3 @@ it("hides the connection string link when feature.disableConnectionStringLogin i 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 1a5dca789..98778ad9d 100644 --- a/src/Platform/Hosted/Components/ConnectExplorer.tsx +++ b/src/Platform/Hosted/Components/ConnectExplorer.tsx @@ -1,9 +1,6 @@ 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 { userContext } from "UserContext"; import * as React from "react"; import ConnectImage from "../../../../images/HdeConnectCosmosDB.svg"; import ErrorImage from "../../../../images/error.svg"; @@ -11,10 +8,9 @@ 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"; +import { isDirectConnectionStringLoginApi } from "../HostedUtils"; interface Props { connectionString: string; @@ -27,10 +23,9 @@ interface Props { 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. +// Turns a failed Portal Backend response into an error that carries the message returned by the service. const errorFromResponse = async (response: Response): Promise => - Object.assign(new Error((await response.text()) || response.statusText), { statusCode: response.status }); + new Error((await response.text()) || response.statusText); export const fetchEncryptedToken = async (connectionString: string): Promise => { const headers = new Headers(); @@ -58,41 +53,6 @@ export const isAccountRestrictedForConnectionStringLogin = async (connectionStri return (await response.text()).toLowerCase() === "true"; }; -// Verifies the connection string can actually authenticate against the account by making a lightweight -// read of the database account through the Cosmos client. A 401 or 403 means the key is invalid or not -// authorized, so the login is blocked and the error returned by the service is shown. Any other failure -// (network blip, throttling, service outage) is not a credential problem, so the login is allowed to -// continue instead of blocking on a transient error. -export const validateDirectConnectionStringConnectivity = async ( - connectionString: string, - metadata: AccessInputMetadata, -): Promise => { - const masterKey = extractMasterKeyfromConnectionString(connectionString); - if (!metadata?.documentEndpoint || !masterKey) { - throw new Error(t(Keys.connectExplorer.errors.connectFailed)); - } - - updateUserContext({ - authType: AuthType.ConnectionString, - masterKey, - endpoint: metadata.documentEndpoint, - refreshCosmosClient: true, - }); - - try { - await client().getDatabaseAccount(); - } catch (error) { - if (isAuthorizationError(error)) { - throw error; - } - - logWarning( - `Could not verify the connection string against the account: ${getErrorMessage(error)}`, - "ConnectExplorer/validateDirectConnectionStringConnectivity", - ); - } -}; - export const ConnectExplorer: React.FunctionComponent = ({ setEncryptedToken, login, @@ -142,13 +102,8 @@ export const ConnectExplorer: React.FunctionComponent = ({ if (metadata && isDirectConnectionStringLoginApi(metadata.apiKind)) { // SQL, Tables, and Gremlin sign data-plane requests client-side with the account key, so // we skip the Portal Backend proxy and use the metadata parsed from the connection string. - try { - await validateDirectConnectionStringConnectivity(connectionString, metadata); - setAccountMetadata(metadata); - setAuthType(AuthType.ConnectionString); - } catch (error) { - setErrorMessage(getErrorMessage(error)); - } + setAccountMetadata(metadata); + setAuthType(AuthType.ConnectionString); return; } diff --git a/src/Platform/Hosted/Helpers/ConnectionStringParser.test.ts b/src/Platform/Hosted/Helpers/ConnectionStringParser.test.ts index c8ded18db..c35a571c8 100644 --- a/src/Platform/Hosted/Helpers/ConnectionStringParser.test.ts +++ b/src/Platform/Hosted/Helpers/ConnectionStringParser.test.ts @@ -16,6 +16,16 @@ describe("ConnectionStringParser", () => { expect(metadata.apiEndpoint).toBeUndefined(); }); + it("should keep the document endpoint given by the connection string", () => { + // The endpoint is taken from the connection string rather than rebuilt from the account name, so a + // string that omits the port keeps it omitted. + const metadata = parseConnectionString( + `AccountEndpoint=https://${mockAccountName}.documents.azure.com/;AccountKey=${mockMasterKey};`, + ); + + expect(metadata.documentEndpoint).toBe(`https://${mockAccountName}.documents.azure.com/`); + }); + it("should parse a valid mongo account connection string", () => { const metadata = parseConnectionString( `mongodb://${mockAccountName}:${mockMasterKey}@${mockAccountName}.documents.azure.com:10255`, diff --git a/src/Platform/Hosted/Helpers/ConnectionStringParser.ts b/src/Platform/Hosted/Helpers/ConnectionStringParser.ts index e8270b868..3f7954e29 100644 --- a/src/Platform/Hosted/Helpers/ConnectionStringParser.ts +++ b/src/Platform/Hosted/Helpers/ConnectionStringParser.ts @@ -2,7 +2,8 @@ import * as Constants from "../../../Common/Constants"; import { AccessInputMetadata, ApiKind } from "../../../Contracts/DataModels"; // Cosmos DB DNS zones used to construct endpoints client-side. These mirror what the Portal Backend's -// accessinputmetadata API constructs from the account name for SQL, Tables, and Gremlin accounts. +// accessinputmetadata API constructs from the account name when the connection string does not already +// contain the endpoint. const DocumentEndpointZone = "documents.azure.com"; const GremlinEndpointZone = "gremlin.cosmos.azure.com"; const DnsPort = "443"; @@ -17,6 +18,9 @@ export function parseConnectionString(connectionString: string): AccessInputMeta if (RegExp(Constants.EndpointsRegex.sql).test(connectionStringPart)) { accessInput.accountName = connectionStringPart.match(Constants.EndpointsRegex.sql)[1]; accessInput.apiKind = ApiKind.SQL; + // SQL and Gremlin connection strings carry the account's document endpoint, so take it as + // given instead of rebuilding it from the account name. + accessInput.documentEndpoint = connectionStringPart.substring(connectionStringPart.indexOf("=") + 1); } else if (RegExp(Constants.EndpointsRegex.mongo).test(connectionStringPart)) { const matches: string[] = connectionStringPart.match(Constants.EndpointsRegex.mongo); accessInput.accountName = matches && matches.length > 1 && matches[2]; @@ -44,18 +48,14 @@ export function parseConnectionString(connectionString: string): AccessInputMeta return undefined; } - // For the APIs that log in directly through the Cosmos client (SQL, Tables, Gremlin), derive the - // endpoints client-side instead of Portal Backend's accessinputmetadata call. + // Tables connection strings only carry the table endpoint, so the document endpoint that data + // operations go through has to be derived from the account name. Gremlin accounts additionally + // need the Gremlin endpoint, which is never part of the connection string. if (accessInput.accountName) { - if ( - accessInput.apiKind === ApiKind.SQL || - accessInput.apiKind === ApiKind.Table || - accessInput.apiKind === ApiKind.Graph - ) { + if (accessInput.apiKind === ApiKind.Table) { accessInput.documentEndpoint = `https://${accessInput.accountName}.${DocumentEndpointZone}:${DnsPort}/`; - if (accessInput.apiKind === ApiKind.Graph) { - accessInput.apiEndpoint = `${accessInput.accountName}.${GremlinEndpointZone}:${DnsPort}`; - } + } else if (accessInput.apiKind === ApiKind.Graph) { + accessInput.apiEndpoint = `${accessInput.accountName}.${GremlinEndpointZone}:${DnsPort}`; } } diff --git a/src/Utils/AuthorizationUtils.test.ts b/src/Utils/AuthorizationUtils.test.ts index 960516520..fd361ac22 100644 --- a/src/Utils/AuthorizationUtils.test.ts +++ b/src/Utils/AuthorizationUtils.test.ts @@ -203,38 +203,4 @@ 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 2d3e92edf..0938c6f33 100644 --- a/src/Utils/AuthorizationUtils.ts +++ b/src/Utils/AuthorizationUtils.ts @@ -254,14 +254,3 @@ 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 89e3ed914..50936a296 100644 --- a/test/sql/connectionStringLogin.spec.ts +++ b/test/sql/connectionStringLogin.spec.ts @@ -83,53 +83,17 @@ test.describe("SQL account using connection string login", () => { expect(resultData?.id).toEqual(documentId); }); - test("shows an error when the connection string has the wrong account key", async ({ page }) => { - // 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. + test("opens Data Explorer when the connection string has the wrong account key", async ({ page }) => { + // A well-formed but incorrect base64 account key. The login is accepted + // without checking the key against the account, so the user gets into Data Explorer either way and + // only the data-plane requests made from inside the explorer are rejected. const wrongKey = "A".repeat(86) + "=="; await loginWithConnectionString(page, `AccountEndpoint=${documentEndpoint};AccountKey=${wrongKey};`); - // 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, - }); - }); + await DataExplorer.waitForExplorer(page); - 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); + // The connect form is replaced by the explorer rather than staying up with a login error. + await expect(page.locator("#connectExplorer")).toHaveCount(0); await expect(page.locator(".errorDetails")).toHaveCount(0); }); });