From 59fc5616ecb1ba28e669dff3798bced488b5ed78 Mon Sep 17 00:00:00 2001 From: "Alex Yang (DevDiv)" Date: Mon, 20 Jul 2026 14:18:50 -0700 Subject: [PATCH] Address connector authentication review feedback Keep terminal sign-in status retry-safe, preserve operational Azure Identity errors, retry transient legacy-cache cleanup, clarify secure persistence requirements, and fix the blocking spelling check. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- extensions/connector-namespaces/README.md | 14 +++- extensions/connector-namespaces/auth.mjs | 21 ++++-- extensions/connector-namespaces/auth.test.mjs | 72 ++++++++++++++++++- extensions/connector-namespaces/renderer.mjs | 4 +- .../connector-namespaces/test/README.md | 9 ++- 5 files changed, 106 insertions(+), 14 deletions(-) diff --git a/extensions/connector-namespaces/README.md b/extensions/connector-namespaces/README.md index ccef0c03..43e7bc99 100644 --- a/extensions/connector-namespaces/README.md +++ b/extensions/connector-namespaces/README.md @@ -36,6 +36,10 @@ and select **Install in GitHub Copilot app**. - Permission to view the namespace and create its connections and hosted MCP server configurations. - A browser for Microsoft Entra sign-in and connector consent. +- An operating-system secure credential store. Windows and macOS provide one by + default. Linux and WSL require a Secret Service-compatible keyring, such as + GNOME Keyring, with `libsecret` available. Unencrypted token storage is + intentionally disabled. Connector Namespace is currently an Azure preview service and availability can vary by region. @@ -60,9 +64,13 @@ playground. Use **Change namespace** to switch subscriptions or namespaces. Azure sign-in and connector sign-in are separate: - **Azure sign-in** lets the canvas discover and manage Connector Namespace - resources. The account cache is stored in the operating system's encrypted - credential store; raw access and refresh tokens are not written to extension - files. + resources. Access and refresh tokens are stored in the operating system's + encrypted credential store. To select that encrypted cache entry after an app + restart, the extension separately saves a non-secret authentication record + containing the authority, client ID, account ID, tenant ID, and username under + `~/.copilot/extensions/connector-namespaces/artifacts/azure-auth-record.json`, + with user-only permissions where supported. Raw tokens are never written to + extension files. - **Connector sign-in** grants an individual MCP server access to its backing service. The resulting connection is managed by Connector Namespace. diff --git a/extensions/connector-namespaces/auth.mjs b/extensions/connector-namespaces/auth.mjs index e5475016..06ea6494 100644 --- a/extensions/connector-namespaces/auth.mjs +++ b/extensions/connector-namespaces/auth.mjs @@ -129,7 +129,13 @@ export class InteractiveAuthBroker { ensureLegacyCredentialsRemoved() { if (!this.cleanupInFlight) { - this.cleanupInFlight = Promise.resolve().then(() => this.cleanupLegacyCredentials()); + const cleanup = Promise.resolve() + .then(() => this.cleanupLegacyCredentials()) + .catch((error) => { + if (this.cleanupInFlight === cleanup) this.cleanupInFlight = null; + throw error; + }); + this.cleanupInFlight = cleanup; } return this.cleanupInFlight; } @@ -198,7 +204,6 @@ export class InteractiveAuthBroker { if (!session) return { ok: false, status: "unknown" }; if (session.status === "pending") return { ok: true, status: "pending", mode: "interactive" }; - this.sessions.delete(sessionId); if (session.status === "done") return { ok: true, status: "done" }; if (session.status === "cancelled") return { ok: true, status: "cancelled" }; return { ok: false, status: "error", error: session.error || "Azure sign-in failed." }; @@ -224,10 +229,13 @@ export class InteractiveAuthBroker { credential = this.createInteractiveCredential(authenticationRecord); this.credential = credential; } catch (error) { - throw new ConnectorAuthenticationRequiredError( - "Azure sign-in is unavailable. Check the secure token cache installation and try again.", - { cause: error }, - ); + if (isAuthenticationRequiredError(error)) { + throw new ConnectorAuthenticationRequiredError( + "Sign in to Azure to continue.", + { cause: error }, + ); + } + throw error; } } const request = credential.getToken(this.scope) @@ -239,6 +247,7 @@ export class InteractiveAuthBroker { return accessToken.token; }) .catch((error) => { + if (!isAuthenticationRequiredError(error)) throw error; if (this.credential === credential) { this.credential = null; this.accessToken = null; diff --git a/extensions/connector-namespaces/auth.test.mjs b/extensions/connector-namespaces/auth.test.mjs index 38191bae..c96e649f 100644 --- a/extensions/connector-namespaces/auth.test.mjs +++ b/extensions/connector-namespaces/auth.test.mjs @@ -96,6 +96,7 @@ test("interactive sign-in reports pending then done and caches the ARM token", a }); assert.equal(authenticateOptions.abortSignal.aborted, false); assert.deepEqual(broker.getSignInStatus(started.sessionId), { ok: true, status: "done" }); + assert.deepEqual(broker.getSignInStatus(started.sessionId), { ok: true, status: "done" }); assert.equal(await broker.getToken(), "token"); }); @@ -150,7 +151,9 @@ test("cancelling sign-in aborts the credential request", async () => { }); }, async getToken() { - throw new Error("getToken should not run after cancellation"); + const error = new Error("No cached account found."); + error.name = "AuthenticationRequiredError"; + throw error; }, }; const broker = new InteractiveAuthBroker({ @@ -170,6 +173,7 @@ test("cancelling sign-in aborts the credential request", async () => { assert.equal(abortSignal.aborted, true); assert.deepEqual(broker.getSignInStatus(started.sessionId), { ok: true, status: "cancelled" }); + assert.deepEqual(broker.getSignInStatus(started.sessionId), { ok: true, status: "cancelled" }); await assert.rejects(broker.getToken(), ConnectorAuthenticationRequiredError); }); @@ -195,4 +199,70 @@ test("sign-in failures are surfaced through the status endpoint contract", async status: "error", error: "browser launch failed", }); + assert.deepEqual(broker.getSignInStatus(started.sessionId), { + ok: false, + status: "error", + error: "browser launch failed", + }); +}); + +test("legacy credential cleanup retries after a transient failure", async () => { + let cleanupCalls = 0; + const broker = new InteractiveAuthBroker({ + cleanupLegacyCredentials: async () => { + cleanupCalls++; + if (cleanupCalls === 1) throw new Error("legacy cache is locked"); + }, + createCredential: () => ({ + async getToken() { + return accessToken(); + }, + }), + loadAuthRecord: async () => authenticationRecord(), + }); + + await assert.rejects(broker.getToken(), /legacy cache is locked/); + assert.equal(await broker.getToken(), "token"); + assert.equal(cleanupCalls, 2); +}); + +test("token acquisition preserves operational errors and retries the credential", async () => { + const outage = new Error("Azure Identity network request timed out"); + let createCredentialCalls = 0; + let tokenCalls = 0; + const broker = new InteractiveAuthBroker({ + createCredential: () => { + createCredentialCalls++; + return { + async getToken() { + tokenCalls++; + if (tokenCalls === 1) throw outage; + return accessToken(); + }, + }; + }, + loadAuthRecord: async () => authenticationRecord(), + }); + + await assert.rejects(broker.getToken(), (error) => error === outage); + assert.equal(await broker.getToken(), "token"); + assert.equal(createCredentialCalls, 1); + assert.equal(tokenCalls, 2); +}); + +test("incomplete tokens remain operational errors instead of prompting sign-in", async () => { + const broker = new InteractiveAuthBroker({ + createCredential: () => ({ + async getToken() { + return { token: "incomplete" }; + }, + }), + loadAuthRecord: async () => authenticationRecord(), + }); + + await assert.rejects( + broker.getToken(), + (error) => !(error instanceof ConnectorAuthenticationRequiredError) + && error.message === "Azure identity returned an incomplete ARM access token.", + ); }); diff --git a/extensions/connector-namespaces/renderer.mjs b/extensions/connector-namespaces/renderer.mjs index ed5094be..a6cdf413 100644 --- a/extensions/connector-namespaces/renderer.mjs +++ b/extensions/connector-namespaces/renderer.mjs @@ -468,7 +468,7 @@ async function loadSubscriptions(force) { } catch (error) { setupContent.hidden = false; signinPanel.hidden = true; - gatewayList.innerHTML = '
Couldn\\'t load subscriptions: ' + escH(error.message) + '
'; + gatewayList.innerHTML = '
Unable to load subscriptions: ' + escH(error.message) + '
'; document.getElementById("retry-subscriptions").addEventListener("click", () => loadSubscriptions(true)); return false; } @@ -515,7 +515,7 @@ async function startSignin() { signinPanel.dataset.state = "error"; signinPanel.setAttribute("aria-busy", "false"); signinButton.disabled = false; - signinMessage.textContent = "Couldn\\'t start sign-in: " + error.message; + signinMessage.textContent = "Unable to start sign-in: " + error.message; } } diff --git a/extensions/connector-namespaces/test/README.md b/extensions/connector-namespaces/test/README.md index aa886c40..ce68026f 100644 --- a/extensions/connector-namespaces/test/README.md +++ b/extensions/connector-namespaces/test/README.md @@ -9,7 +9,7 @@ connect → initialize → tools/list → a safe tools/call It imports the `connector-namespaces` extension's real pipeline (`install.mjs`, `catalog.mjs`, `armClient.mjs`) and connects through the same native Streamable -HTTP endpoint that the extension writes to the Copilot CLI config. The probe +HTTP endpoint that the extension writes to the GitHub Copilot MCP config. The probe uses the configured `X-API-Key`, follows `Mcp-Session-Id`, and accepts standard JSON or SSE JSON-RPC responses. @@ -27,6 +27,11 @@ MCP server issue locally. (`{ subscriptionId, resourceGroup, gatewayName }`). Pick a gateway once in the connector-namespaces canvas, or write that file by hand. 3. **Node 20+** (developed on Node 24). +4. **Extension dependencies installed.** From the repository root, run: + + ```bash + npm install --prefix extensions/connector-namespaces + ``` ## Run it @@ -70,7 +75,7 @@ connector's own consent. The model: loopback page is just a redirect target and nothing is listening on it. 3. **Re-run the harness.** It sees the pending record, confirms the gateway connection is now `Connected`, finishes the install (mints the API key, - writes the CLI entry), and probes it headless. From then on the connector is + writes the Copilot MCP entry), and probes it headless. From then on the connector is reused without repeating its consent. So the server taxonomy is: