Skip to content

Improve connector reload authentication UX #2370

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

Open
alexyaang wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Improve connector reload authentication UX #2370

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 9 additions & 13 deletions extensions/connector-namespaces/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ and connected-server management into the Copilot side panel.
- **My MCPs** - see which servers are connected and ready to add to Copilot.
- **Namespace playground** - open any connected server in the Connector
Namespace playground with **Sandbox**.
- **Persistent setup** - retain the selected namespace and restore Azure sign-in
securely across app restarts.
- **Persistent namespace selection** - retain the selected namespace while Azure
tokens remain in memory and sign-in is requested again after a restart.

## Install

Expand All @@ -36,10 +36,6 @@ 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.
Expand All @@ -64,13 +60,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. 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.
resources. Access and refresh tokens remain in the extension process and are
never written to extension files. Reloading the extension or restarting the
app requires Azure sign-in again. The selected namespace coordinates are
retained in
`~/.copilot/extensions/connector-namespaces/artifacts/gateway-config.json` so
the canvas can explain that the namespace is still linked and return directly
to its connectors after sign-in.
- **Connector sign-in** grants an individual MCP server access to its backing
service. The resulting connection is managed by Connector Namespace.

Expand Down
88 changes: 16 additions & 72 deletions extensions/connector-namespaces/auth.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,9 @@ import { randomUUID } from "node:crypto";
import { promises as fs } from "node:fs";
import { homedir } from "node:os";
import { join } from "node:path";
import {
deserializeAuthenticationRecord,
InteractiveBrowserCredential,
serializeAuthenticationRecord,
useIdentityPlugin,
} from "@azure/identity";
import { cachePersistencePlugin } from "@azure/identity-cache-persistence";
import { InteractiveBrowserCredential } from "@azure/identity";

export const ARM_SCOPE = "https://management.azure.com/.default";
export const TOKEN_CACHE_NAME = "github-copilot-connector-namespaces";

const TOKEN_EXPIRY_SKEW_MS = 5 * 60 * 1000;
const SIGN_IN_SESSION_TTL_MS = 10 * 60 * 1000;
Expand All @@ -24,50 +17,20 @@ const AUTH_STORAGE_DIR = join(
const AUTH_RECORD_FILE = join(AUTH_STORAGE_DIR, "azure-auth-record.json");
const LEGACY_AUTH_CACHE = join(AUTH_STORAGE_DIR, "auth-cache.json");

let legacyAuthCacheRemoved = false;
let legacyAuthArtifactsRemoved = false;

useIdentityPlugin(cachePersistencePlugin);

export async function loadAuthenticationRecord({
readFile = fs.readFile,
deserialize = deserializeAuthenticationRecord,
authRecordFile = AUTH_RECORD_FILE,
} = {}) {
let serialized;
try {
serialized = await readFile(authRecordFile, "utf-8");
} catch (error) {
if (error?.code === "ENOENT") return undefined;
throw error;
}
try {
return deserialize(serialized);
} catch {
return undefined;
}
}

async function saveAuthenticationRecord(record) {
await fs.mkdir(AUTH_STORAGE_DIR, { recursive: true, mode: 0o700 });
await fs.chmod(AUTH_STORAGE_DIR, 0o700);
await fs.writeFile(
AUTH_RECORD_FILE,
serializeAuthenticationRecord(record),
{ encoding: "utf-8", mode: 0o600 },
);
await fs.chmod(AUTH_RECORD_FILE, 0o600);
}

async function removeLegacyAuthCache() {
if (legacyAuthCacheRemoved) return;
try {
await fs.unlink(LEGACY_AUTH_CACHE);
} catch (error) {
if (error?.code !== "ENOENT") {
throw new Error(`Could not remove the legacy connector credential cache at ${LEGACY_AUTH_CACHE}: ${error.message}`);
async function removeLegacyAuthArtifacts() {
if (legacyAuthArtifactsRemoved) return;
for (const path of [AUTH_RECORD_FILE, LEGACY_AUTH_CACHE]) {
try {
await fs.unlink(path);
} catch (error) {
if (error?.code !== "ENOENT") {
throw new Error(`Could not remove the legacy connector authentication file at ${path}: ${error.message}`);
}
}
}
legacyAuthCacheRemoved = true;
legacyAuthArtifactsRemoved = true;
}

export class ConnectorAuthenticationRequiredError extends Error {
Expand Down Expand Up @@ -103,36 +66,25 @@ export class InteractiveAuthBroker {
createCredential = credentialFactory,
createSessionId = randomUUID,
cleanupLegacyCredentials = async () => {},
loadAuthRecord = async () => undefined,
saveAuthRecord = async () => {},
now = Date.now,
scope = ARM_SCOPE,
tokenCacheName = TOKEN_CACHE_NAME,
} = {}) {
this.createCredential = createCredential;
this.createSessionId = createSessionId;
this.cleanupLegacyCredentials = cleanupLegacyCredentials;
this.loadAuthRecord = loadAuthRecord;
this.saveAuthRecord = saveAuthRecord;
this.now = now;
this.scope = scope;
this.tokenCacheName = tokenCacheName;
this.credential = null;
this.accessToken = null;
this.cleanupInFlight = null;
this.tokenInFlight = null;
this.sessions = new Map();
}

createInteractiveCredential(authenticationRecord) {
createInteractiveCredential() {
return this.createCredential({
redirectUri: "http://localhost",
disableAutomaticAuthentication: true,
tokenCachePersistenceOptions: {
enabled: true,
name: this.tokenCacheName,
},
...(authenticationRecord ? { authenticationRecord } : {}),
});
}

Expand Down Expand Up @@ -188,7 +140,6 @@ export class InteractiveAuthBroker {
if (!authenticationRecord) {
throw new Error("Azure identity did not return an authentication record.");
}
await this.saveAuthRecord(authenticationRecord);
const accessToken = await credential.getToken(this.scope, { abortSignal: abortController.signal });
if (!accessToken?.token || !Number.isFinite(accessToken.expiresOnTimestamp)) {
throw new Error("Azure identity returned an incomplete ARM access token.");
Expand Down Expand Up @@ -235,13 +186,8 @@ export class InteractiveAuthBroker {
let credential = this.credential;
if (!credential) {
try {
const authenticationRecord = await this.loadAuthRecord();
if (hasUsableToken(this.accessToken, this.now())) return this.accessToken.token;
credential = this.credential;
if (!credential) {
credential = this.createInteractiveCredential(authenticationRecord);
this.credential = credential;
}
credential = this.createInteractiveCredential();
this.credential = credential;
} catch (error) {
if (isAuthenticationRequiredError(error)) {
throw new ConnectorAuthenticationRequiredError(
Expand Down Expand Up @@ -281,9 +227,7 @@ export class InteractiveAuthBroker {
}

export const interactiveAuth = new InteractiveAuthBroker({
cleanupLegacyCredentials: removeLegacyAuthCache,
loadAuthRecord: loadAuthenticationRecord,
saveAuthRecord: saveAuthenticationRecord,
cleanupLegacyCredentials: removeLegacyAuthArtifacts,
});

export const startSignIn = () => interactiveAuth.startSignIn();
Expand Down
81 changes: 22 additions & 59 deletions extensions/connector-namespaces/auth.test.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ import assert from "node:assert/strict";
import {
ConnectorAuthenticationRequiredError,
InteractiveAuthBroker,
loadAuthenticationRecord,
TOKEN_CACHE_NAME,
} from "./auth.mjs";

function accessToken(token = "token", expiresOnTimestamp = 2_000_000_000_000) {
Expand Down Expand Up @@ -35,40 +33,20 @@ test("ARM token requests require an explicit browser sign-in", async () => {
},
};
},
loadAuthRecord: async () => undefined,
});

await assert.rejects(
broker.getToken(),
(error) => error instanceof ConnectorAuthenticationRequiredError
&& error.code === "authentication_required",
);
assert.deepEqual(credentialOptions.tokenCachePersistenceOptions, {
enabled: true,
name: TOKEN_CACHE_NAME,
assert.deepEqual(credentialOptions, {
redirectUri: "http://localhost",
disableAutomaticAuthentication: true,
});
});

test("a malformed authentication record falls back to browser sign-in", async () => {
assert.equal(
await loadAuthenticationRecord({
readFile: async () => "{malformed-json",
}),
undefined,
);
});

test("authentication record read failures remain operational errors", async () => {
const readError = Object.assign(new Error("authentication record is unreadable"), { code: "EACCES" });
await assert.rejects(
loadAuthenticationRecord({
readFile: async () => { throw readError; },
}),
(error) => error === readError,
);
});

test("interactive sign-in reports pending then done and caches the ARM token", async () => {
test("interactive sign-in reports pending then done and keeps the ARM token in memory", async () => {
let credentialOptions;
let authenticateOptions;
const credential = {
Expand All @@ -89,7 +67,6 @@ test("interactive sign-in reports pending then done and caches the ARM token", a
return credential;
},
createSessionId: () => "signin-session",
saveAuthRecord: async (record) => assert.deepEqual(record, authenticationRecord()),
now: () => 1_000,
});

Expand All @@ -110,10 +87,6 @@ test("interactive sign-in reports pending then done and caches the ARM token", a
assert.deepEqual(credentialOptions, {
redirectUri: "http://localhost",
disableAutomaticAuthentication: true,
tokenCachePersistenceOptions: {
enabled: true,
name: TOKEN_CACHE_NAME,
},
});
assert.equal(authenticateOptions.abortSignal.aborted, false);
assert.deepEqual(broker.getSignInStatus(started.sessionId), { ok: true, status: "done" });
Expand All @@ -124,22 +97,24 @@ test("interactive sign-in reports pending then done and caches the ARM token", a
assert.equal(await broker.getToken(), "token");
});

test("a new broker restores the persisted credential without reopening the browser", async () => {
const cache = { record: null, token: null };
test("a new broker requires browser sign-in after the extension reloads", async () => {
let authenticateCalls = 0;
let createCredentialCalls = 0;
const createCredential = (options) => {
assert.deepEqual(options.tokenCachePersistenceOptions, {
enabled: true,
name: TOKEN_CACHE_NAME,
createCredentialCalls++;
assert.deepEqual(options, {
redirectUri: "http://localhost",
disableAutomaticAuthentication: true,
});
let signedIn = false;
return {
async authenticate() {
authenticateCalls++;
cache.token = accessToken("persisted-token");
signedIn = true;
return authenticationRecord();
},
async getToken() {
if (cache.token && options.authenticationRecord) return cache.token;
if (signedIn) return accessToken("memory-token");
const error = new Error("No cached account found.");
error.name = "AuthenticationRequiredError";
throw error;
Expand All @@ -149,41 +124,36 @@ test("a new broker restores the persisted credential without reopening the brows
const signedInBroker = new InteractiveAuthBroker({
createCredential,
createSessionId: () => "persist-session",
saveAuthRecord: async (record) => { cache.record = record; },
now: () => 1_000,
});
const started = signedInBroker.startSignIn();
await signedInBroker.sessions.get(started.sessionId).promise;
assert.equal(authenticateCalls, 1);
assert.equal(await signedInBroker.getToken(), "memory-token");

const restartedBroker = new InteractiveAuthBroker({
createCredential,
loadAuthRecord: async () => cache.record,
now: () => 1_000,
});
assert.equal(await restartedBroker.getToken(), "persisted-token");
await assert.rejects(restartedBroker.getToken(), ConnectorAuthenticationRequiredError);
assert.equal(authenticateCalls, 1);
assert.equal(createCredentialCalls, 2);
});

test("concurrent first-time token requests share credential initialization and acquisition", async () => {
let releaseAuthenticationRecord;
const authenticationRecordReady = new Promise((resolve) => {
releaseAuthenticationRecord = resolve;
test("concurrent first-time token requests share credential acquisition", async () => {
let releaseToken;
const tokenReady = new Promise((resolve) => {
releaseToken = resolve;
});
let loadAuthRecordCalls = 0;
let createCredentialCalls = 0;
let tokenCalls = 0;
const broker = new InteractiveAuthBroker({
async loadAuthRecord() {
loadAuthRecordCalls++;
await authenticationRecordReady;
return authenticationRecord();
},
createCredential: () => {
createCredentialCalls++;
return {
async getToken() {
tokenCalls++;
await tokenReady;
return accessToken("shared-token");
},
};
Expand All @@ -193,15 +163,12 @@ test("concurrent first-time token requests share credential initialization and a
const firstRequest = broker.getToken();
const secondRequest = broker.getToken();
await new Promise((resolve) => setImmediate(resolve));
const loadsBeforeRelease = loadAuthRecordCalls;
releaseAuthenticationRecord();
releaseToken();

assert.deepEqual(
await Promise.all([firstRequest, secondRequest]),
["shared-token", "shared-token"],
);
assert.equal(loadsBeforeRelease, 1);
assert.equal(loadAuthRecordCalls, 1);
assert.equal(createCredentialCalls, 1);
assert.equal(tokenCalls, 1);
});
Expand All @@ -224,7 +191,6 @@ test("cancelling sign-in aborts the credential request", async () => {
const broker = new InteractiveAuthBroker({
createCredential: () => credential,
createSessionId: () => "cancel-session",
loadAuthRecord: async () => undefined,
now: () => 1_000,
});

Expand Down Expand Up @@ -289,7 +255,6 @@ test("legacy credential cleanup retries after a transient failure", async () =>
return accessToken();
},
}),
loadAuthRecord: async () => authenticationRecord(),
});

await assert.rejects(broker.getToken(), /legacy cache is locked/);
Expand All @@ -312,7 +277,6 @@ test("token acquisition preserves operational errors and retries the credential"
},
};
},
loadAuthRecord: async () => authenticationRecord(),
});

await assert.rejects(broker.getToken(), (error) => error === outage);
Expand All @@ -328,7 +292,6 @@ test("incomplete tokens remain operational errors instead of prompting sign-in",
return { token: "incomplete" };
},
}),
loadAuthRecord: async () => authenticationRecord(),
});

await assert.rejects(
Expand Down
Loading
Loading