Skip to content

Commit f3e8ccb

Browse files
TylerLeonhardtCopilot
TylerLeonhardt
and
Copilot
authored
Add client ID fallback for MCP OAuth when Dynamic Client Registration fails (microsoft#255633)
* Initial plan * Add client ID fallback functionality for MCP OAuth when DCR fails Co-authored-by: TylerLeonhardt <[email protected]> * Add client secret support for MCP OAuth when DCR fails Co-authored-by: TylerLeonhardt <[email protected]> * Merge client ID and client secret prompts into single $promptForClientDetails method Co-authored-by: TylerLeonhardt <[email protected]> * Add persistent client secret storage for MCP OAuth authentication Co-authored-by: TylerLeonhardt <[email protected]> * Consolidate client credentials storage in SecretStorage Co-authored-by: TylerLeonhardt <[email protected]> * Consolidate getClientId and getClientSecret into single getClientDetails method Co-authored-by: TylerLeonhardt <[email protected]> * Add modal dialog and improve UX for client registration prompts - Add modal dialog before prompting for client details to explain DCR not supported - Include redirect URI information in modal dialog (http://localhost:*, https://vscode.dev/redirect) - Use shared title "Add Client Registration Details" across multi-step quick picks - Remove "please" from prompts to keep them concise - Remove unnecessary validateInput function for client secret - Improve overall user experience flow Co-authored-by: TylerLeonhardt <[email protected]> * misc fixes * polish * No longer need to fill in urls --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]>
1 parent ce8f8ed commit f3e8ccb

File tree

10 files changed

+258
-86
lines changed

10 files changed

+258
-86
lines changed

‎src/vs/base/common/oauth.ts

Lines changed: 1 addition & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -258,12 +258,6 @@ export interface IAuthorizationServerMetadata {
258258
code_challenge_methods_supported?: string[];
259259
}
260260

261-
export interface IRequiredAuthorizationServerMetadata extends IAuthorizationServerMetadata {
262-
authorization_endpoint: string;
263-
token_endpoint: string;
264-
registration_endpoint: string;
265-
}
266-
267261
/**
268262
* Response from the dynamic client registration endpoint.
269263
*/
@@ -697,7 +691,7 @@ export function isAuthorizationRegistrationErrorResponse(obj: unknown): obj is I
697691

698692
//#endregion
699693

700-
export function getDefaultMetadataForUrl(authorizationServer: URL): IRequiredAuthorizationServerMetadata & IRequiredAuthorizationServerMetadata {
694+
export function getDefaultMetadataForUrl(authorizationServer: URL): IAuthorizationServerMetadata {
701695
return {
702696
issuer: authorizationServer.toString(),
703697
authorization_endpoint: new URL('/authorize', authorizationServer).toString(),
@@ -709,16 +703,6 @@ export function getDefaultMetadataForUrl(authorizationServer: URL): IRequiredAut
709703
};
710704
}
711705

712-
export function getMetadataWithDefaultValues(metadata: IAuthorizationServerMetadata): IAuthorizationServerMetadata & IRequiredAuthorizationServerMetadata {
713-
const issuer = new URL(metadata.issuer);
714-
return {
715-
...metadata,
716-
authorization_endpoint: metadata.authorization_endpoint ?? new URL('/authorize', issuer).toString(),
717-
token_endpoint: metadata.token_endpoint ?? new URL('/token', issuer).toString(),
718-
registration_endpoint: metadata.registration_endpoint ?? new URL('/register', issuer).toString(),
719-
};
720-
}
721-
722706
/**
723707
* The grant types that we support
724708
*/

‎src/vs/base/test/common/oauth.test.ts

Lines changed: 47 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import * as sinon from 'sinon';
88
import {
99
getClaimsFromJWT,
1010
getDefaultMetadataForUrl,
11-
getMetadataWithDefaultValues,
1211
getResourceServerBaseUrlFromDiscoveryUrl,
1312
isAuthorizationAuthorizeResponse,
1413
isAuthorizationDeviceResponse,
@@ -189,37 +188,6 @@ suite('OAuth', () => {
189188
assert.strictEqual(metadata.registration_endpoint, 'https://auth.example.com/register');
190189
assert.deepStrictEqual(metadata.response_types_supported, ['code', 'id_token', 'id_token token']);
191190
});
192-
193-
test('getMetadataWithDefaultValues should fill in missing endpoints', () => {
194-
const minimal: IAuthorizationServerMetadata = {
195-
issuer: 'https://auth.example.com',
196-
response_types_supported: ['code']
197-
};
198-
199-
const complete = getMetadataWithDefaultValues(minimal);
200-
201-
assert.strictEqual(complete.issuer, 'https://auth.example.com');
202-
assert.strictEqual(complete.authorization_endpoint, 'https://auth.example.com/authorize');
203-
assert.strictEqual(complete.token_endpoint, 'https://auth.example.com/token');
204-
assert.strictEqual(complete.registration_endpoint, 'https://auth.example.com/register');
205-
assert.deepStrictEqual(complete.response_types_supported, ['code']);
206-
});
207-
208-
test('getMetadataWithDefaultValues should preserve custom endpoints', () => {
209-
const custom: IAuthorizationServerMetadata = {
210-
issuer: 'https://auth.example.com',
211-
authorization_endpoint: 'https://auth.example.com/custom-authorize',
212-
token_endpoint: 'https://auth.example.com/custom-token',
213-
registration_endpoint: 'https://auth.example.com/custom-register',
214-
response_types_supported: ['code', 'token']
215-
};
216-
217-
const complete = getMetadataWithDefaultValues(custom);
218-
219-
assert.strictEqual(complete.authorization_endpoint, 'https://auth.example.com/custom-authorize');
220-
assert.strictEqual(complete.token_endpoint, 'https://auth.example.com/custom-token');
221-
assert.strictEqual(complete.registration_endpoint, 'https://auth.example.com/custom-register');
222-
});
223191
});
224192

225193
suite('Parsing Functions', () => {
@@ -756,4 +724,51 @@ suite('OAuth', () => {
756724
assert.strictEqual(result, 'https://mcp.example.com/');
757725
});
758726
});
727+
728+
suite('Client ID Fallback Scenarios', () => {
729+
let sandbox: sinon.SinonSandbox;
730+
let fetchStub: sinon.SinonStub;
731+
732+
setup(() => {
733+
sandbox = sinon.createSandbox();
734+
fetchStub = sandbox.stub(globalThis, 'fetch');
735+
});
736+
737+
teardown(() => {
738+
sandbox.restore();
739+
});
740+
741+
test('fetchDynamicRegistration should throw specific error for missing registration endpoint', async () => {
742+
const serverMetadata: IAuthorizationServerMetadata = {
743+
issuer: 'https://auth.example.com',
744+
response_types_supported: ['code']
745+
// registration_endpoint is missing
746+
};
747+
748+
await assert.rejects(
749+
async () => await fetchDynamicRegistration(serverMetadata, 'Test Client'),
750+
{
751+
message: 'Server does not support dynamic registration'
752+
}
753+
);
754+
});
755+
756+
test('fetchDynamicRegistration should throw specific error for DCR failure', async () => {
757+
fetchStub.resolves({
758+
ok: false,
759+
text: async () => 'DCR not supported'
760+
} as Response);
761+
762+
const serverMetadata: IAuthorizationServerMetadata = {
763+
issuer: 'https://auth.example.com',
764+
registration_endpoint: 'https://auth.example.com/register',
765+
response_types_supported: ['code']
766+
};
767+
768+
await assert.rejects(
769+
async () => await fetchDynamicRegistration(serverMetadata, 'Test Client'),
770+
/Registration to https:\/\/auth\.example\.com\/register failed: DCR not supported/
771+
);
772+
});
773+
});
759774
});

‎src/vs/workbench/api/browser/mainThreadAuthentication.ts

Lines changed: 69 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import { DeferredPromise, raceTimeout } from '../../../base/common/async.js';
2727
import { IAuthorizationTokenResponse } from '../../../base/common/oauth.js';
2828
import { IDynamicAuthenticationProviderStorageService } from '../../services/authentication/common/dynamicAuthenticationProviderStorage.js';
2929
import { IClipboardService } from '../../../platform/clipboard/common/clipboardService.js';
30+
import { IQuickInputService } from '../../../platform/quickinput/common/quickInput.js';
3031

3132
export interface AuthenticationInteractiveOptions {
3233
detail?: string;
@@ -94,7 +95,8 @@ export class MainThreadAuthentication extends Disposable implements MainThreadAu
9495
@ILogService private readonly logService: ILogService,
9596
@IURLService private readonly urlService: IURLService,
9697
@IDynamicAuthenticationProviderStorageService private readonly dynamicAuthProviderStorageService: IDynamicAuthenticationProviderStorageService,
97-
@IClipboardService private readonly clipboardService: IClipboardService
98+
@IClipboardService private readonly clipboardService: IClipboardService,
99+
@IQuickInputService private readonly quickInputService: IQuickInputService
98100
) {
99101
super();
100102
this._proxy = extHostContext.getProxy(ExtHostContext.ExtHostAuthentication);
@@ -121,7 +123,9 @@ export class MainThreadAuthentication extends Disposable implements MainThreadAu
121123
create: async (authorizationServer, serverMetadata, resource) => {
122124
// Auth Provider Id is a combination of the authorization server and the resource, if provided.
123125
const authProviderId = resource ? `${authorizationServer.toString(true)} ${resource.resource}` : authorizationServer.toString(true);
124-
const clientId = this.dynamicAuthProviderStorageService.getClientId(authProviderId);
126+
const clientDetails = await this.dynamicAuthProviderStorageService.getClientRegistration(authProviderId);
127+
const clientId = clientDetails?.clientId;
128+
const clientSecret = clientDetails?.clientSecret;
125129
let initialTokens: (IAuthorizationTokenResponse & { created_at: number })[] | undefined = undefined;
126130
if (clientId) {
127131
initialTokens = await this.dynamicAuthProviderStorageService.getSessionsForDynamicAuthProvider(authProviderId, clientId);
@@ -131,6 +135,7 @@ export class MainThreadAuthentication extends Disposable implements MainThreadAu
131135
serverMetadata,
132136
resource,
133137
clientId,
138+
clientSecret,
134139
initialTokens
135140
);
136141
}
@@ -224,25 +229,28 @@ export class MainThreadAuthentication extends Disposable implements MainThreadAu
224229
return deferredPromise.p;
225230
}
226231

227-
async $registerDynamicAuthenticationProvider(id: string, label: string, authorizationServer: UriComponents, clientId: string): Promise<void> {
232+
async $registerDynamicAuthenticationProvider(id: string, label: string, authorizationServer: UriComponents, clientId: string, clientSecret?: string): Promise<void> {
228233
await this.$registerAuthenticationProvider(id, label, true, [authorizationServer]);
229-
this.dynamicAuthProviderStorageService.storeClientId(id, URI.revive(authorizationServer).toString(true), clientId, label);
234+
await this.dynamicAuthProviderStorageService.storeClientRegistration(id, URI.revive(authorizationServer).toString(true), clientId, clientSecret, label);
230235
}
231236

232237
async $setSessionsForDynamicAuthProvider(authProviderId: string, clientId: string, sessions: (IAuthorizationTokenResponse & { created_at: number })[]): Promise<void> {
233238
await this.dynamicAuthProviderStorageService.setSessionsForDynamicAuthProvider(authProviderId, clientId, sessions);
234239
}
235240

236-
async $sendDidChangeDynamicProviderInfo({ providerId, clientId, authorizationServer, label }: Partial<{ providerId: string; clientId: string; authorizationServer: UriComponents; label: string }>): Promise<void> {
241+
async $sendDidChangeDynamicProviderInfo({ providerId, clientId, authorizationServer, label, clientSecret }: Partial<{ providerId: string; clientId: string; authorizationServer: UriComponents; label: string; clientSecret: string }>): Promise<void> {
237242
this.logService.info(`Client ID for authentication provider ${providerId} changed to ${clientId}`);
238243
const existing = this.dynamicAuthProviderStorageService.getInteractedProviders().find(p => p.providerId === providerId);
239244
if (!existing) {
240245
throw new Error(`Dynamic authentication provider ${providerId} not found. Has it been registered?`);
241246
}
242-
this.dynamicAuthProviderStorageService.storeClientId(
247+
248+
// Store client credentials together
249+
await this.dynamicAuthProviderStorageService.storeClientRegistration(
243250
providerId || existing.providerId,
244251
authorizationServer ? URI.revive(authorizationServer).toString(true) : existing.authorizationServer,
245252
clientId || existing.clientId,
253+
clientSecret,
246254
label || existing.label
247255
);
248256
}
@@ -536,4 +544,59 @@ export class MainThreadAuthentication extends Disposable implements MainThreadAu
536544
}
537545
return false;
538546
}
547+
548+
async $promptForClientRegistration(authorizationServerUrl: string): Promise<{ clientId: string; clientSecret?: string } | undefined> {
549+
// Show modal dialog first to explain the situation and get user consent
550+
const result = await this.dialogService.prompt({
551+
type: Severity.Info,
552+
message: nls.localize('dcrNotSupported', "Dynamic Client Registration not supported"),
553+
detail: nls.localize('dcrNotSupportedDetail', "The authorization server '{0}' does not support automatic client registration. Do you want to proceed by manually providing a client registration (client ID)?\n\nNote: When registering your OAuth application, make sure to include these redirect URIs:\nhttp://127.0.0.1:33418\nhttps://vscode.dev/redirect", authorizationServerUrl),
554+
buttons: [
555+
{
556+
label: nls.localize('provideClientDetails', "Proceed"),
557+
run: () => true
558+
}
559+
],
560+
cancelButton: {
561+
label: nls.localize('cancel', "Cancel"),
562+
run: () => false
563+
}
564+
});
565+
566+
if (!result) {
567+
return undefined;
568+
}
569+
570+
const sharedTitle = nls.localize('addClientRegistrationDetails', "Add Client Registration Details");
571+
572+
const clientId = await this.quickInputService.input({
573+
title: sharedTitle,
574+
prompt: nls.localize('clientIdPrompt', "Enter an existing client ID that has been registered with the following redirect URIs: http://127.0.0.1:33418, https://vscode.dev/redirect"),
575+
placeHolder: nls.localize('clientIdPlaceholder', "OAuth client ID (azye39d...)"),
576+
ignoreFocusLost: true,
577+
validateInput: async (value: string) => {
578+
if (!value || value.trim().length === 0) {
579+
return nls.localize('clientIdRequired', "Client ID is required");
580+
}
581+
return undefined;
582+
}
583+
});
584+
585+
if (!clientId || clientId.trim().length === 0) {
586+
return undefined;
587+
}
588+
589+
const clientSecret = await this.quickInputService.input({
590+
title: sharedTitle,
591+
prompt: nls.localize('clientSecretPrompt', "(optional) Enter an existing client secret associated with the client id '{0}' or leave this field blank", clientId),
592+
placeHolder: nls.localize('clientSecretPlaceholder', "OAuth client secret (wer32o50f...) or leave it blank"),
593+
password: true,
594+
ignoreFocusLost: true
595+
});
596+
597+
return {
598+
clientId: clientId.trim(),
599+
clientSecret: clientSecret?.trim() || undefined
600+
};
601+
}
539602
}

‎src/vs/workbench/api/common/extHost.protocol.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -193,9 +193,10 @@ export interface MainThreadAuthenticationShape extends IDisposable {
193193
$waitForUriHandler(expectedUri: UriComponents): Promise<UriComponents>;
194194
$showContinueNotification(message: string): Promise<boolean>;
195195
$showDeviceCodeModal(userCode: string, verificationUri: string): Promise<boolean>;
196-
$registerDynamicAuthenticationProvider(id: string, label: string, authorizationServer: UriComponents, clientId: string): Promise<void>;
196+
$promptForClientRegistration(authorizationServerUrl: string): Promise<{ clientId: string; clientSecret?: string } | undefined>;
197+
$registerDynamicAuthenticationProvider(id: string, label: string, authorizationServer: UriComponents, clientId: string, clientSecret?: string): Promise<void>;
197198
$setSessionsForDynamicAuthProvider(authProviderId: string, clientId: string, sessions: (IAuthorizationTokenResponse & { created_at: number })[]): Promise<void>;
198-
$sendDidChangeDynamicProviderInfo({ providerId, clientId, authorizationServer, label }: { providerId: string; clientId?: string; authorizationServer?: UriComponents; label?: string }): Promise<void>;
199+
$sendDidChangeDynamicProviderInfo({ providerId, clientId, authorizationServer, label, clientSecret }: { providerId: string; clientId?: string; authorizationServer?: UriComponents; label?: string; clientSecret?: string }): Promise<void>;
199200
}
200201

201202
export interface MainThreadSecretStateShape extends IDisposable {
@@ -1988,7 +1989,7 @@ export interface ExtHostAuthenticationShape {
19881989
$removeSession(id: string, sessionId: string): Promise<void>;
19891990
$onDidChangeAuthenticationSessions(id: string, label: string, extensionIdFilter?: string[]): Promise<void>;
19901991
$onDidUnregisterAuthenticationProvider(id: string): Promise<void>;
1991-
$registerDynamicAuthProvider(authorizationServer: UriComponents, serverMetadata: IAuthorizationServerMetadata, resource?: IAuthorizationProtectedResourceMetadata, clientId?: string, initialTokens?: (IAuthorizationTokenResponse & { created_at: number })[]): Promise<string>;
1992+
$registerDynamicAuthProvider(authorizationServer: UriComponents, serverMetadata: IAuthorizationServerMetadata, resource?: IAuthorizationProtectedResourceMetadata, clientId?: string, clientSecret?: string, initialTokens?: (IAuthorizationTokenResponse & { created_at: number })[]): Promise<string>;
19921993
$onDidChangeDynamicAuthProviderTokens(authProviderId: string, clientId: string, tokens?: (IAuthorizationTokenResponse & { created_at: number })[]): Promise<void>;
19931994
}
19941995

0 commit comments

Comments
 (0)