diff --git a/README.md b/README.md index 2b61bed..d9a7d21 100644 --- a/README.md +++ b/README.md @@ -135,6 +135,7 @@ mcpx @add "{ name: '', url: '' }" mcpx @add "{ name: '', url: '', bearer: 'env:' }" mcpx @add "{ name: '', transport: 'stdio', command: '', args: [''] }" mcpx @remove "{ name: '' }" +mcpx @login "{ name: '' }" mcpx @refresh mcpx @daemon.status mcpx @daemon.stop @@ -194,9 +195,13 @@ discovers its authorization server from that challenge, authorizes, records the server as OAuth, and retries the request. This covers servers whose tools are partly anonymous — `@add` lists their tools without credentials, and the first call to a protected tool starts sign-in. A `403 insufficient_scope` -challenge re-authorizes with the additional scope the same way. `@add` itself -never opens a browser; it reports `auth-required` when listing tools was -challenged. +challenge re-authorizes with the additional scope the same way. When `@add` +itself is challenged, it signs in right away and then lists the tools. + +To switch accounts, run `mcpx @login "{ name: '' }"`. It opens a new +browser sign-in (with OIDC `prompt=login`, so the old browser session is not +reused silently) and replaces the stored grant only once the new sign-in +succeeds. For OAuth servers that support dynamic client registration, mcpx registers a client automatically for each browser sign-in. diff --git a/docs/adr/0009-a-challenge-is-the-oauth-discovery-entry.md b/docs/adr/0009-a-challenge-is-the-oauth-discovery-entry.md index 54eeaa6..19851a4 100644 --- a/docs/adr/0009-a-challenge-is-the-oauth-discovery-entry.md +++ b/docs/adr/0009-a-challenge-is-the-oauth-discovery-entry.md @@ -10,7 +10,7 @@ Whether a server needs OAuth is decided by the resource server's challenge, at a `@add` used to `GET` the server URL and freeze the result as the server's auth kind. A mixed-auth server — anonymous tools beside protected ones, such as `whiteboardanimation.ai` — answers that `GET` with `405`, so it was recorded as unauthenticated, and its spec-compliant challenge on a protected tool could only end in `reauth-required`. The probe guessed from the wrong request; the challenge is the server's own answer. -- `@add` no longer probes. It lists tools; a challenged listing records the server as OAuth and reports `auth-required` without opening a browser. +- `@add` no longer probes. It lists tools; a challenged listing records the server as OAuth. (It used to stop there with `auth-required`; [ADR 0010](0010-adding-signs-in-and-login-replaces-a-grant.md) has it sign in.) - The transport carries an mcpx `authProvider` whose `onUnauthorized` surfaces the challenge instead of running OAuth inside the request, so the Runtime keeps single-flight, Caller Input, progress events, and registry ownership (ADR 0002). - OAuth itself — discovery, dynamic registration, PKCE, resource indicators, `iss` validation, refresh — is the official MCP SDK's `auth()`. mcpx supplies its `OAuthClientProvider`: per-server grants in the Credential Store, a loopback callback per sign-in, and the fixed `http://127.0.0.1:65245/callback` for manually registered clients. A refresh never opens the browser; a new sign-in never replays stored tokens or a client registered for another port, and replaces the stored grant only once it succeeds. - A bearer credential that is challenged still fails with `reauth-required`; static credentials cannot be repaired by the Runtime. diff --git a/docs/adr/0010-adding-signs-in-and-login-replaces-a-grant.md b/docs/adr/0010-adding-signs-in-and-login-replaces-a-grant.md new file mode 100644 index 0000000..0f5408a --- /dev/null +++ b/docs/adr/0010-adding-signs-in-and-login-replaces-a-grant.md @@ -0,0 +1,17 @@ +--- +type: Decision +title: Adding Signs In, and @login Replaces a Grant +status: accepted +--- + +# Adding Signs In, and @login Replaces a Grant + +`@add` is the user asking to use a server, so a challenged tool listing goes straight on to the shared Authentication Flow (ADR 0004) and then lists the tools: one command takes an OAuth server from unknown to callable. `mcpx @login "{ name }"` starts a new browser sign-in for a registered OAuth server even when its grant still works, which is how an account is switched. + +[ADR 0009](0009-a-challenge-is-the-oauth-discovery-entry.md) kept `@add` from opening a browser because it was written when only explicit commands could (ADR 0007). [ADR 0008](0008-calls-wait-for-shared-authorization.md) already lets an ordinary Call open one, which left `@add` as the only entry that refused, and made switching accounts a `@remove` + `@add` detour whose only purpose was deleting the stored grant. + +- `@add` still never probes; it lists tools, and only a challenged listing on a server without a configured bearer signs in. A failed sign-in keeps the registration and reports the failure; `@login` retries it. +- A sign-in skips the refresh grant and asks again for the previously granted scope. It replaces the stored grant only once the new one is earned (the existing interactive-mode guarantee), so an abandoned or failed `@login` leaves the old account working. +- The authorization URL carries OIDC `prompt=login`, so a browser still signed in to the old account cannot silently authorize it again. Authorization servers without OIDC ignore the parameter. +- `@login` on a bearer or stdio server fails: their credentials are declared, not earned, and change by re-adding. +- A sign-in that arrives while a refresh flow for the same Credential Identity is running joins that flow (ADR 0004) and gets its result; run `@login` again afterwards. diff --git a/package.json b/package.json index 48e830d..3c28726 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "mcpx", - "version": "0.14.3", + "version": "0.15.0", "license": "MIT", "bin": { "mcpx": "./src/main.ts" diff --git a/src/discovery.ts b/src/discovery.ts index f636514..7bda7a0 100644 --- a/src/discovery.ts +++ b/src/discovery.ts @@ -52,15 +52,15 @@ export async function discoverServer( } } catch (error) { const message = error instanceof Error ? error.message : String(error) - // The challenge, not a probe, says the server wants OAuth (ADR 0009). - // @add never starts OAuth, so name the commands that do. + // The challenge, not a probe, says the server wants OAuth (ADR 0009); the + // Runtime's @add goes on to sign in (ADR 0010). if (!configuredAuth && authorizationChallenge(error)) { server.auth = { kind: 'oauth' } return { server, status: 'auth-required', message: - 'Authentication is required before tool schemas can be listed. Run `mcpx @refresh` or call a tool to authenticate.', + 'Authentication is required before tool schemas can be listed. Run `mcpx @login` to sign in.', } } if (configuredAuth && authorizationChallenge(error)) { diff --git a/src/oauth.ts b/src/oauth.ts index 1a1b55c..2fbbfac 100644 --- a/src/oauth.ts +++ b/src/oauth.ts @@ -59,6 +59,11 @@ export type AuthorizeOAuthOptions = { serverUrl: URL grants: OAuthGrantStore challenge?: OAuthChallenge | undefined + /** + * A new sign-in the user asked for, such as switching accounts: skip the + * refresh grant and ask the authorization server to prompt for login again. + */ + signIn?: boolean signal: AbortSignal requestManualClient: ( request: ManualOAuthClientRequest, @@ -98,14 +103,15 @@ export function oauthAccessToken( export async function authorizeOAuth( options: AuthorizeOAuthOptions, ): Promise { - const { challenge, signal } = options + const { challenge, signal, signIn } = options const grant = await options.grants.read() const grantedScope = grant?.tokens?.value.scope - // A 403 step-up keeps what was granted and adds what the server asked for. + // A 403 step-up keeps what was granted and adds what the server asked for; + // a new sign-in asks again for what the old grant held. const scope = challenge?.status === 403 ? computeScopeUnion(grantedScope, challenge.scope) - : challenge?.scope + : (challenge?.scope ?? (signIn ? grantedScope : undefined)) const widensScope = challenge?.status === 403 && isStrictScopeSuperset(scope, grantedScope) const base = { @@ -118,7 +124,12 @@ export async function authorizeOAuth( } // A refresh grant cannot widen scope (RFC 6749 §6), so step-up skips it. - if (grant?.client && grant.tokens?.value.refresh_token && !widensScope) { + if ( + !signIn && + grant?.client && + grant.tokens?.value.refresh_token && + !widensScope + ) { options.report({ phase: 'refreshing-token', message: 'refreshing OAuth token', @@ -322,6 +333,10 @@ class McpxOAuthProvider implements OAuthClientProvider { redirectToAuthorization(authorizationUrl: URL): void { if (this.#mode === 'refresh') throw new InteractionRequired() + // OIDC `prompt=login`: a browser still signed in to the old account must + // not silently authorize it again. Servers without OIDC ignore it. + if (this.#options.signIn) + authorizationUrl.searchParams.set('prompt', 'login') const url = authorizationUrl.toString() // The Runtime's stderr is detached; callers must receive the URL as an event // so a failed browser launch never turns into a silent wait. diff --git a/src/router.ts b/src/router.ts index 61ca63e..9f6c436 100644 --- a/src/router.ts +++ b/src/router.ts @@ -94,6 +94,12 @@ const removeInput = s( }), ) +const loginInput = s( + v.object({ + name: v.pipe(v.string(), v.description('Registered OAuth server name')), + }), +) + const skillInput = s( v.object({ servers: v.optional( @@ -169,6 +175,13 @@ function buildRouter(registry: RegistryView): Router { ], }) .input(removeInput), + '@login': c + .meta({ + description: + 'Sign in to an OAuth MCP server in the browser, replacing its current grant (e.g. to switch accounts). The old grant stays until the new sign-in succeeds.', + examples: [`mcpx @login "{ name: 'higgsfield' }"`], + }) + .input(loginInput), '@refresh': c.meta({ description: 'Refresh all registered MCP server schemas and report auth status.', @@ -331,7 +344,26 @@ function buildHandlers( if (bearer) intent.bearer = bearer } return await renderOutput( - await requestRuntime(intent, mainPath), + await requestRuntime(intent, mainPath, { + onEvent: renderRuntimeProgress, + onInput: promptForRuntimeInput, + }), + options.context, + ) + } + + handlers['@login'] = async (options: HandlerOptions<{ name: string }>) => { + return await renderOutput( + await requestRuntime( + { + requestId: crypto.randomUUID(), + op: 'refreshServers', + serverNames: [assertServerName(options.input.name)], + signIn: true, + }, + mainPath, + { onEvent: renderRuntimeProgress, onInput: promptForRuntimeInput }, + ), options.context, ) } diff --git a/src/runtime-authentication.ts b/src/runtime-authentication.ts index bccb12c..ef36302 100644 --- a/src/runtime-authentication.ts +++ b/src/runtime-authentication.ts @@ -24,6 +24,8 @@ export type AuthorizationProgress = AuthenticationProgress & { export type AuthorizeOptions = { /** The server rejected a request; its challenge says what it wants. */ challenge?: OAuthChallenge | undefined + /** Replace the grant with a new browser sign-in, even if it still works. */ + signIn?: boolean signal?: AbortSignal onProgress?: (progress: AuthorizationProgress) => void } @@ -77,20 +79,29 @@ export class RuntimeAuthentication { `Unknown MCP server: ${serverName}.`, ) } + const { challenge, signIn } = options + if ( + signIn && + (server.transport === 'stdio' || server.auth.kind === 'bearer') + ) + throw new RuntimeOperationError( + 'operation-failed', + `${serverName} does not sign in through OAuth; re-add it with new credentials.`, + ) if (server.transport === 'stdio') return 'ready' - const { challenge } = options if (server.auth.kind === 'bearer') { // Static credentials cannot be repaired by the Runtime. if (challenge) throw reauthRequired(serverName) return 'ready' } - if (server.auth.kind === 'none' && !challenge) return 'ready' + if (server.auth.kind === 'none' && !challenge && !signIn) return 'ready' const observed = credentials.oauth[serverName] if ( server.auth.kind === 'oauth' && !challenge && + !signIn && oauthAccessToken(observed) ) return 'ready' @@ -101,7 +112,9 @@ export class RuntimeAuthentication { ] // A caller can arrive after a shared flow persisted but before it observed // completion; reuse that rotation instead of authorizing again. + // A sign-in wants a new grant, never one that merely rotated. if ( + !signIn && current?.tokens?.value.access_token !== observed?.tokens?.value.access_token && oauthAccessToken(current) @@ -112,6 +125,7 @@ export class RuntimeAuthentication { serverUrl: new URL(server.url), grants: this.#grantStore(serverName), challenge, + signIn, signal, report, openBrowser: this.#openBrowser, diff --git a/src/runtime-protocol.ts b/src/runtime-protocol.ts index ca37f8c..60809d0 100644 --- a/src/runtime-protocol.ts +++ b/src/runtime-protocol.ts @@ -81,7 +81,13 @@ export type RuntimeIntent = env?: Record } | { requestId: string; op: 'removeServers'; serverNames: string[] } - | { requestId: string; op: 'refreshServers'; serverNames?: string[] } + | { + requestId: string + op: 'refreshServers' + serverNames?: string[] + /** Replace each OAuth grant with a new browser sign-in. */ + signIn?: boolean + } | { requestId: string; op: 'status' } | { requestId: string; op: 'stop' } @@ -176,9 +182,10 @@ export function parseRuntimeIntent( ? (value as RuntimeIntent) : invalidFrame() case 'refreshServers': - return hasOnly(value, ['requestId', 'op', 'serverNames']) && + return hasOnly(value, ['requestId', 'op', 'serverNames', 'signIn']) && hasRequired(value, ['requestId', 'op']) && - (value.serverNames === undefined || isStringArray(value.serverNames)) + (value.serverNames === undefined || isStringArray(value.serverNames)) && + (value.signIn === undefined || typeof value.signIn === 'boolean') ? (value as RuntimeIntent) : invalidFrame() default: diff --git a/src/runtime.ts b/src/runtime.ts index a6d9a75..9c401ab 100644 --- a/src/runtime.ts +++ b/src/runtime.ts @@ -87,7 +87,11 @@ export class McpRuntime { return case 'refreshServers': try { - const outcome = await this.#refreshServers(intent.serverNames, caller) + const outcome = await this.#refreshServers( + intent.serverNames, + caller, + { signIn: intent.signIn === true }, + ) if (outcome.status === 'disconnected') return await caller.send({ requestId: intent.requestId, @@ -106,20 +110,48 @@ export class McpRuntime { try { const result = await discoverServer(discoveryOptions(intent)) await this.#stores.upsertServer(intent.serverName, result.server) + const added = { + name: intent.serverName, + transport: result.server.transport ?? 'http', + status: result.status, + auth: + result.server.transport === 'stdio' + ? undefined + : result.server.auth, + tools: result.server.tools?.length ?? 0, + message: result.message, + } + // Adding is the user asking to use the server: a challenged listing + // goes straight on to browser sign-in, then lists the tools (ADR 0010). + if ( + result.status === 'auth-required' && + result.server.transport !== 'stdio' && + result.server.auth.kind === 'oauth' + ) { + const outcome = await this.#refreshServers( + [intent.serverName], + caller, + { signIn: false }, + ) + if (outcome.status === 'disconnected') return + const failure = outcome.failed[0] + const snapshot = await this.#stores.readSnapshot() + Object.assign( + added, + failure + ? { message: failure.message } + : { + status: 'ready', + tools: + snapshot.servers[intent.serverName]?.tools?.length ?? 0, + message: 'Signed in and discovered tools.', + }, + ) + } await caller.send({ requestId: intent.requestId, kind: 'result', - result: { - name: intent.serverName, - transport: result.server.transport ?? 'http', - status: result.status, - auth: - result.server.transport === 'stdio' - ? undefined - : result.server.auth, - tools: result.server.tools?.length ?? 0, - message: result.message, - }, + result: added, }) } catch (error) { await caller.send({ @@ -166,6 +198,7 @@ export class McpRuntime { async #refreshServers( serverNames: string[] | undefined, caller: RuntimeCaller, + options: { signIn: boolean }, ): Promise { const registry = await this.#stores.registry.read() const names = serverNames ?? Object.keys(registry.servers).sort() @@ -215,6 +248,7 @@ export class McpRuntime { name, caller, { + signIn: options.signIn, onProgress: (progress) => report(name, progress.phase, progress.message, progress.url), }, diff --git a/src/skill-template.ts b/src/skill-template.ts index cec8dbc..b84db8d 100644 --- a/src/skill-template.ts +++ b/src/skill-template.ts @@ -279,6 +279,9 @@ mcpx @refresh If the current session is not a TTY, ask the user to run it in their terminal. +To sign in with a different account, run +\`mcpx @login "{ name: '' }"\` and share the printed URL with the user. + Once \`mcpx\` lists the server and a focused \`@schema\` call succeeds, return to the skill. ` diff --git a/tests/oauth-challenge.test.ts b/tests/oauth-challenge.test.ts index 5e124d8..d4ffde4 100644 --- a/tests/oauth-challenge.test.ts +++ b/tests/oauth-challenge.test.ts @@ -132,6 +132,68 @@ describe('OAuth discovered from a call-time challenge', () => { ) }) + it('signs in during @add, then @login replaces the working grant', async () => { + const fixture = serveMixedAuthServer({ protectAll: true }) + const root = await fs.mkdtemp(path.join(tmpdir(), 'mcpx-signin-')) + roots.push(root) + const stores = await openRuntimeStores(root) + const browserUrls: string[] = [] + const runtime = new McpRuntime(stores, { + authentication: new RuntimeAuthentication(stores, { + openBrowser: (url) => { + browserUrls.push(url) + void fetch(url).catch(() => {}) + }, + }), + }) + + const add = createInMemoryRuntimeCaller('add') + await runtime.handle( + { + requestId: 'add', + op: 'addServer', + serverName: 'wb', + transport: 'http', + url: fixture.url, + }, + add, + ) + expect(add.frames.find((frame) => frame.kind === 'result')).toMatchObject({ + result: { status: 'ready', auth: { kind: 'oauth' }, tools: 2 }, + }) + expect(browserUrls).toHaveLength(1) + expect(new URL(browserUrls[0]!).searchParams.get('prompt')).toBe(null) + + // The grant still works; a refresh leaves it alone, a sign-in does not. + const refresh = createInMemoryRuntimeCaller('refresh') + await runtime.handle( + { requestId: 'refresh', op: 'refreshServers', serverNames: ['wb'] }, + refresh, + ) + expect(browserUrls).toHaveLength(1) + const login = createInMemoryRuntimeCaller('login') + await runtime.handle( + { + requestId: 'login', + op: 'refreshServers', + serverNames: ['wb'], + signIn: true, + }, + login, + ) + expect(login.frames.find((frame) => frame.kind === 'result')).toMatchObject( + { + result: { status: 'completed', refreshed: ['wb'], failed: [] }, + }, + ) + expect(browserUrls).toHaveLength(2) + expect(new URL(browserUrls[1]!).searchParams.get('prompt')).toBe('login') + await runtime.handle( + { requestId: 'stop', op: 'stop' }, + createInMemoryRuntimeCaller('stop'), + ) + }) + function serveMixedAuthServer( options: { protectAll?: boolean; insufficientScope?: boolean } = {}, ) { diff --git a/tests/oauth.test.ts b/tests/oauth.test.ts index 7af29e7..b383310 100644 --- a/tests/oauth.test.ts +++ b/tests/oauth.test.ts @@ -91,6 +91,34 @@ describe('oauth', () => { ]) }) + it('signs in again instead of refreshing a usable grant, prompting for login', async () => { + const fixture = serveAuthorizationServer() + const stored = storedGrant(fixture.origin) + stored.tokens!.value.scope = 'mcp:tools' + const flow = flowOptions(fixture, { grant: stored, signIn: true }) + + await authorizeOAuth(flow.options) + + expect(fixture.tokenRequests.map((body) => body.get('grant_type'))).toEqual( + ['authorization_code'], + ) + const authorize = new URL(flow.browserUrls[0]!) + expect(authorize.searchParams.get('prompt')).toBe('login') + expect(authorize.searchParams.get('scope')).toBe('mcp:tools') + expect(flow.grant?.tokens?.value.access_token).toBe('access-1') + expect(flow.grant?.client?.information.client_id).toBe('dynamic-client') + }) + + it('keeps the old grant when a new sign-in fails', async () => { + const fixture = serveAuthorizationServer({ callbackIssuer: 'https://evil' }) + const stored = storedGrant(fixture.origin) + const flow = flowOptions(fixture, { grant: stored, signIn: true }) + + await expect(authorizeOAuth(flow.options)).rejects.toThrow() + + expect(flow.grant).toEqual(stored) + }) + it('falls back to browser authorization when the refresh grant is revoked', async () => { const fixture = serveAuthorizationServer({ rejectRefresh: true }) const flow = flowOptions(fixture, { grant: storedGrant(fixture.origin) }) @@ -305,6 +333,7 @@ describe('oauth', () => { overrides: { grant?: OAuthGrant challenge?: AuthorizeOAuthOptions['challenge'] + signIn?: boolean requestManualClient?: AuthorizeOAuthOptions['requestManualClient'] } = {}, ) { @@ -324,6 +353,7 @@ describe('oauth', () => { }, }, challenge: overrides.challenge, + ...(overrides.signIn ? { signIn: true } : {}), signal: new AbortController().signal, report: (progress) => flow.progress.push(progress), requestManualClient: diff --git a/tests/router.test.ts b/tests/router.test.ts index edb2c3a..31ce4b4 100644 --- a/tests/router.test.ts +++ b/tests/router.test.ts @@ -11,6 +11,7 @@ describe('router', () => { expect(Object.keys(router).sort()).toEqual([ '@add', '@daemon', + '@login', '@refresh', '@remove', '@skill-install',