Skip to content
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.

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

Revert "feat: support extending McpServer with authorization" #272

Merged
merged 1 commit into from
Apr 7, 2025

Conversation

jspahrsummers
Copy link
Member

@jspahrsummers jspahrsummers commented Apr 7, 2025

Reverts #249, as I don't think the core changes of that PR are a good idea:

  1. I think it's generally going to be a bad idea to subclass McpServer, and it will definitely cause problems (now or in the future) to override the way it installs tool call handlers, etc. Two possible alternatives here:
    1. Either these kinds of customizations should go into the callbacks given to McpServer.tool, or
    2. This should be done via composition rather than inheritance.
  2. Adding a writable property Transport.user of unknown type—which isn't set anywhere in the SDK—isn't useful, and is actively confusing because it provides no semantics or type information. Since object types are open-ended in JS, the correct approach would be to just save an additional property, and use a TS declaration in your application to refer to that property. Something like this:
declare module "@modelcontextprotocol/sdk" {
  declare class StdioServerTransport {
    user?: User;
  }
}

If there are ways in which these suggestions prevent something from being possible, let's discuss, but I suspect there's no additional SDK support required here.

@jspahrsummers jspahrsummers merged commit 44839ee into main Apr 7, 2025
4 checks passed
@jspahrsummers jspahrsummers deleted the revert-249-main branch April 7, 2025 10:17
@cliffhall
Copy link
Contributor

I think it's generally going to be a bad idea to subclass McpServer, and it will definitely cause problems (now or in the future) to override the way it installs tool call handlers, etc. Two possible alternatives here:
Either these kinds of customizations should go into the callbacks given to McpServer.tool, or
This should be done via composition rather than inheritance.

@jspahrsummers Should we put this advice to put in the header of McpServer.ts, as I it isn't obvious that subclassing is considered to be a bad approach for working with this class.

@jspahrsummers
Copy link
Member Author

Sure, we can do, although this is really more of a general OO principles thing.

@cliffhall
Copy link
Contributor

general OO principles thing

Agreed, but there is a choice available to the implementor, who may not always choose composition. In the case they do want to use inheritance, some guidance in this class's docs could head off efforts such as @wangshijun's in the future.

@wangshijun
Copy link
Contributor

wangshijun commented Apr 10, 2025

Sure, we can do, although this is really more of a general OO principles thing.

Agree with the "composition over inheritance" principal, 👍

I took a look at the codebase for the latest version and noticed that there's no easy way to support following common use case:

  • Our system has predefined access control policies for MCP tools, similar to role-based access control.
  • We need to verify user permissions before each MCP tool call to ensure security.

It would be great if the McpServer could provide the authorization context to each tool callback at runtime. This way, each tool would know who's making the request and could perform a permission check before diving into the actual business logic. Here's an example of how I imagine it working:

import { ZodRawShape } from 'zod';
import { McpServer, ToolCallback } from './server/mcp.js';
import { RequestHandlerExtra } from './shared/protocol.js';
import { JSONRPCRequest, ServerNotification, ServerRequest } from './types.js';

const mcpServer = new McpServer({
  name: 'test server with auth',
  version: '1.0',
});

type SessionUser = {
  role: string;
  [key: string]: unknown;
};

type AccessPolicy = {
  allow?: {
    roles?: string[];
  };
  deny?: {
    roles?: string[];
  };
};

type AuthorizeContext = {
  user?: SessionUser;
  ip?: string;
  ua?: string;
};

function checkPermission(request: JSONRPCRequest, authorizeContext: AuthorizeContext, policy?: AccessPolicy): boolean {
  // do permission check base on request, authorizeContext and policy
  return true;
}

function wrapToolCallback(cb: ToolCallback<ZodRawShape>, policy?: AccessPolicy) {
  return (async (
    request: JSONRPCRequest,
    extra: RequestHandlerExtra<ServerRequest, ServerNotification>,
    authorizeContext: AuthorizeContext
  ) => {
    const hasPermission = await checkPermission(request, authorizeContext, policy);
    if (!hasPermission) {
      throw new Error('Unauthorized');
    }
    return cb(request, extra);
  }) as ToolCallback<ZodRawShape>;
}

mcpServer.tool(
  'protected-tool',
  wrapToolCallback(
    () => ({
      content: [
        {
          type: 'text',
          text: 'Protected tool response',
        },
      ],
    }),
    {
      allow: {
        roles: ['admin'],
      },
    }
  )
);

@jspahrsummers @cliffhall Do you have any thoughts or suggestions on the use case mentioned above? 😊

@jspahrsummers
Copy link
Member Author

You might be able to make use of #166 once merged, which I think would be a clean way to support something like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants